| Message ID | 1405088313-20048-6-git-send-email-mperttunen@nvidia.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote: > Add binding documentation for the nvidia,tegra124-emc device tree > node. > > Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> > --- > .../bindings/memory-controllers/tegra-emc.txt | 42 ++++++++++++++++++++++ > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt > > diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt > new file mode 100644 > index 0000000..2dde17e > --- /dev/null > +++ b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt > @@ -0,0 +1,42 @@ > +Tegra124 SoC EMC controller > + > +Required properties : > +- compatible : "nvidia,tegra124-emc". > +- reg : Should contain 1 or 2 entries: > + - EMC register set > + - MC register set : Required only if no node with > + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set > + is first read from the MC node. If it doesn't exist, it is read > + from this property. No can do. Memory regions shouldn't be shared between drivers like this. It makes it impossible to ensure that they don't stump on each others' toes. One possibility to make this work is to export global functions from the memory controller driver that this driver can call into. Perhaps if you want you can be extra explicit by linking them in DT, like this: mc: memory-controller@0,70019000 { compatible = "nvidia,tegra124-mc"; reg = <0x0 0x70019000 0x0 0x00001000>; ... }; memory-controller@0,7001b000 { compatible = "nvidia,tegra124-emc"; reg = <0x0 0x7001b000 0x0 0x00001000>; memory-controller = <&mc>; ... }; But I think it's safe enough to assume that there will only be a single memory controller/EMC pair in the device. Thierry
On 07/11/2014 05:51 PM, Thierry Reding wrote: > On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote: >> Add binding documentation for the nvidia,tegra124-emc device tree >> node. >> >> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> >> --- >> .../bindings/memory-controllers/tegra-emc.txt | 42 ++++++++++++++++++++++ >> 1 file changed, 42 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt >> >> diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt >> new file mode 100644 >> index 0000000..2dde17e >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt >> @@ -0,0 +1,42 @@ >> +Tegra124 SoC EMC controller >> + >> +Required properties : >> +- compatible : "nvidia,tegra124-emc". >> +- reg : Should contain 1 or 2 entries: >> + - EMC register set >> + - MC register set : Required only if no node with >> + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set >> + is first read from the MC node. If it doesn't exist, it is read >> + from this property. > > No can do. Memory regions shouldn't be shared between drivers like this. > It makes it impossible to ensure that they don't stump on each others' > toes. In this case, all the registers that will be written are such that the MC driver will never need to write them. They are shadowed registers, meaning that all writes are stored and are only effective starting from the next time the EMC rate change state machine is activated, so writing them from anywhere except than the EMC driver would be pointless. I can find two users of these registers in downstream: 1) mc.c saves and loads them on suspend/restore (I don't know why, that shouldn't do anything. They will be overridden anyway during the next EMC rate change). 2) tegra12x_la.c reads MC_EMEM_ARB_MISC0 during a core_initcall to calculate a value which it then writes to a register that is also shadowed and that is part of downstream burst registers so that doesn't do anything either. The reason I implemented two ways to specify the MC register area was that this could be merged before an MC driver and retain backwards-compatibility after the MC driver arrives. If this is not acceptable, we can certainly wait for the MC driver to be merged first. (Although with the general rate of things, I hope I won't be back at school at that point..) I assume that this is blocked on the IOMMU bindings discussion? In that case, there are several options: the MC driver could have its own tables for each EMC rate or we could just make the EMC tables global (i.e. not under the EMC node). In any case, the MC driver would need to implement a function that would just write these values but be guaranteed to not do anything else, since that could cause nasty things during the EMC rate change sequence. Yet another option is to just not write to these registers at all. In my tests, that would entail a 20-25% penalty to memory throughput for most timings. > > One possibility to make this work is to export global functions from the > memory controller driver that this driver can call into. Perhaps if you > want you can be extra explicit by linking them in DT, like this: > > mc: memory-controller@0,70019000 { > compatible = "nvidia,tegra124-mc"; > reg = <0x0 0x70019000 0x0 0x00001000>; > ... > }; > > memory-controller@0,7001b000 { > compatible = "nvidia,tegra124-emc"; > reg = <0x0 0x7001b000 0x0 0x00001000>; > memory-controller = <&mc>; > ... > }; > > But I think it's safe enough to assume that there will only be a single > memory controller/EMC pair in the device. > > Thierry > -- 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 Fri, Jul 11, 2014 at 7:18 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote: > Add binding documentation for the nvidia,tegra124-emc device tree > node. > diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt > +Required properties : > +- compatible : "nvidia,tegra124-emc". > +- reg : Should contain 1 or 2 entries: > + - EMC register set > + - MC register set : Required only if no node with > + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set > + is first read from the MC node. If it doesn't exist, it is read > + from this property. > +- timings : Should contain 1 entry for each supported clock rate. > + Entries should be named "timing@n" where n is a 0-based increasing > + number. The timings must be listed in rate-ascending order. There are upcoming boards which support multiple DRAM configurations and require a separate set of timings for each configuration. Could we instead have multiple sets of timings with the proper one selected at runtime by RAM code, as reported by PMC_STRAPPING_OPT_A_0? Something like: emc { emc-table@0 { nvidia,ram-code = <0>; timing@0 { ... }; ... }; emc-table@1 { nvidia,ram-code = <4>; timing@0 { ... }; ... }; ... }; -- 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
Sure, I'll do that for v2. On 07/11/2014 07:43 PM, Andrew Bresticker wrote: > On Fri, Jul 11, 2014 at 7:18 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote: >> Add binding documentation for the nvidia,tegra124-emc device tree >> node. > >> diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt > >> +Required properties : >> +- compatible : "nvidia,tegra124-emc". >> +- reg : Should contain 1 or 2 entries: >> + - EMC register set >> + - MC register set : Required only if no node with >> + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set >> + is first read from the MC node. If it doesn't exist, it is read >> + from this property. >> +- timings : Should contain 1 entry for each supported clock rate. >> + Entries should be named "timing@n" where n is a 0-based increasing >> + number. The timings must be listed in rate-ascending order. > > There are upcoming boards which support multiple DRAM configurations > and require a separate set of timings for each configuration. Could > we instead have multiple sets of timings with the proper one selected > at runtime by RAM code, as reported by PMC_STRAPPING_OPT_A_0? > Something like: > > emc { > emc-table@0 { > nvidia,ram-code = <0>; > timing@0 { > ... > }; > ... > }; > emc-table@1 { > nvidia,ram-code = <4>; > timing@0 { > ... > }; > ... > }; > ... > }; > -- > 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 > -- 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 11/07/14 19:01, Mikko Perttunen wrote: > On 07/11/2014 05:51 PM, Thierry Reding wrote: >> On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote: >>> ... >> ... > > In this case, all the registers that will be written are such that the > MC driver will never need to write them. They are shadowed registers, > meaning that all writes are stored and are only effective starting from > the next time the EMC rate change state machine is activated, so writing > them from anywhere except than the EMC driver would be pointless. > > I can find two users of these registers in downstream: > 1) mc.c saves and loads them on suspend/restore (I don't know why, that > shouldn't do anything. They will be overridden anyway during the next > EMC rate change). > 2) tegra12x_la.c reads MC_EMEM_ARB_MISC0 during a core_initcall to > calculate a value which it then writes to a register that is also > shadowed and that is part of downstream burst registers so that doesn't > do anything either. > > The reason I implemented two ways to specify the MC register area was > that this could be merged before an MC driver and retain > backwards-compatibility after the MC driver arrives. > > If this is not acceptable, we can certainly wait for the MC driver to be > merged first. (Although with the general rate of things, I hope I won't > be back at school at that point..) I assume that this is blocked on the > IOMMU bindings discussion? In that case, there are several options: the > MC driver could have its own tables for each EMC rate or we could just > make the EMC tables global (i.e. not under the EMC node). In any case, > the MC driver would need to implement a function that would just write > these values but be guaranteed to not do anything else, since that could > cause nasty things during the EMC rate change sequence. Having taken another look at the code, I don't think the MC driver could do anything that bad. There are also two other places where the EMC driver needs to read MC registers: Inside the sequence, it reads a register but discards its contents. According to comments, this acts as a memory barrier, probably for the preceding step that writes into MC memory. If the register writes are moved to the MC driver, it could also handle that. In another place it reads the number of RAM modules from a MC register. The MC driver could export this as another function. That said, I still suspect it might not be worth it to divide this between two drivers. - Mikko -- 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 Mon, Jul 14, 2014 at 10:55:51AM +0300, Mikko Perttunen wrote: > On 11/07/14 19:01, Mikko Perttunen wrote: > >On 07/11/2014 05:51 PM, Thierry Reding wrote: > >>On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote: > >>>... > >>... > > > >In this case, all the registers that will be written are such that the > >MC driver will never need to write them. They are shadowed registers, > >meaning that all writes are stored and are only effective starting from > >the next time the EMC rate change state machine is activated, so writing > >them from anywhere except than the EMC driver would be pointless. > > > >I can find two users of these registers in downstream: > >1) mc.c saves and loads them on suspend/restore (I don't know why, that > >shouldn't do anything. They will be overridden anyway during the next > >EMC rate change). > >2) tegra12x_la.c reads MC_EMEM_ARB_MISC0 during a core_initcall to > >calculate a value which it then writes to a register that is also > >shadowed and that is part of downstream burst registers so that doesn't > >do anything either. > > > >The reason I implemented two ways to specify the MC register area was > >that this could be merged before an MC driver and retain > >backwards-compatibility after the MC driver arrives. > > > >If this is not acceptable, we can certainly wait for the MC driver to be > >merged first. (Although with the general rate of things, I hope I won't > >be back at school at that point..) I assume that this is blocked on the > >IOMMU bindings discussion? In that case, there are several options: the > >MC driver could have its own tables for each EMC rate or we could just > >make the EMC tables global (i.e. not under the EMC node). In any case, > >the MC driver would need to implement a function that would just write > >these values but be guaranteed to not do anything else, since that could > >cause nasty things during the EMC rate change sequence. > > Having taken another look at the code, I don't think the MC driver could do > anything that bad. There are also two other places where the EMC driver > needs to read MC registers: Inside the sequence, it reads a register but > discards its contents. According to comments, this acts as a memory barrier, > probably for the preceding step that writes into MC memory. If the register > writes are moved to the MC driver, it could also handle that. In another > place it reads the number of RAM modules from a MC register. The MC driver > could export this as another function. Exporting this functionality from the MC driver is the right thing to do in my opinion. > That said, I still suspect it might not be worth it to divide this between > two drivers. If we don't separate, then we make it easy for people to break things in the future. Both drivers may not be accessing the same registers now, but if there's no barrier in place to actively enforce this split, then it's only a matter of time before it gets broken. A typical way to ensure this is done properly is via request_resource() (called by the devm_ioremap_resource() function). That will fail if two drivers try to use the same memory region, and with good reason. Thierry
On 14/07/14 11:15, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, Jul 14, 2014 at 10:55:51AM +0300, Mikko Perttunen wrote: >> On 11/07/14 19:01, Mikko Perttunen wrote: >>> On 07/11/2014 05:51 PM, Thierry Reding wrote: >>>> On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote: >>>>> ... >>>> ... >>> >>> In this case, all the registers that will be written are such that the >>> MC driver will never need to write them. They are shadowed registers, >>> meaning that all writes are stored and are only effective starting from >>> the next time the EMC rate change state machine is activated, so writing >>> them from anywhere except than the EMC driver would be pointless. >>> >>> I can find two users of these registers in downstream: >>> 1) mc.c saves and loads them on suspend/restore (I don't know why, that >>> shouldn't do anything. They will be overridden anyway during the next >>> EMC rate change). >>> 2) tegra12x_la.c reads MC_EMEM_ARB_MISC0 during a core_initcall to >>> calculate a value which it then writes to a register that is also >>> shadowed and that is part of downstream burst registers so that doesn't >>> do anything either. >>> >>> The reason I implemented two ways to specify the MC register area was >>> that this could be merged before an MC driver and retain >>> backwards-compatibility after the MC driver arrives. >>> >>> If this is not acceptable, we can certainly wait for the MC driver to be >>> merged first. (Although with the general rate of things, I hope I won't >>> be back at school at that point..) I assume that this is blocked on the >>> IOMMU bindings discussion? In that case, there are several options: the >>> MC driver could have its own tables for each EMC rate or we could just >>> make the EMC tables global (i.e. not under the EMC node). In any case, >>> the MC driver would need to implement a function that would just write >>> these values but be guaranteed to not do anything else, since that could >>> cause nasty things during the EMC rate change sequence. >> >> Having taken another look at the code, I don't think the MC driver could do >> anything that bad. There are also two other places where the EMC driver >> needs to read MC registers: Inside the sequence, it reads a register but >> discards its contents. According to comments, this acts as a memory barrier, >> probably for the preceding step that writes into MC memory. If the register >> writes are moved to the MC driver, it could also handle that. In another >> place it reads the number of RAM modules from a MC register. The MC driver >> could export this as another function. > > Exporting this functionality from the MC driver is the right thing to do > in my opinion. Ok, let's do that then. Do you think I could make a bare-bones MC driver to support the EMC driver before your MC driver with IOMMU/LA is ready? Can the MC device tree node be stabilized yet? Of course, if things go well, that concern might turn out to be unnecessary. Thanks, Mikko. > >> That said, I still suspect it might not be worth it to divide this between >> two drivers. > > If we don't separate, then we make it easy for people to break things in > the future. Both drivers may not be accessing the same registers now, > but if there's no barrier in place to actively enforce this split, then > it's only a matter of time before it gets broken. A typical way to > ensure this is done properly is via request_resource() (called by the > devm_ioremap_resource() function). That will fail if two drivers try to > use the same memory region, and with good reason. > > Thierry > > * Unknown Key > * 0x7F3EB3A1 > -- 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 Mon, Jul 14, 2014 at 12:06:32PM +0300, Mikko Perttunen wrote: > On 14/07/14 11:15, Thierry Reding wrote: > >* PGP Signed by an unknown key > > > >On Mon, Jul 14, 2014 at 10:55:51AM +0300, Mikko Perttunen wrote: > >>On 11/07/14 19:01, Mikko Perttunen wrote: > >>>On 07/11/2014 05:51 PM, Thierry Reding wrote: > >>>>On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote: > >>>>>... > >>>>... > >>> > >>>In this case, all the registers that will be written are such that the > >>>MC driver will never need to write them. They are shadowed registers, > >>>meaning that all writes are stored and are only effective starting from > >>>the next time the EMC rate change state machine is activated, so writing > >>>them from anywhere except than the EMC driver would be pointless. > >>> > >>>I can find two users of these registers in downstream: > >>>1) mc.c saves and loads them on suspend/restore (I don't know why, that > >>>shouldn't do anything. They will be overridden anyway during the next > >>>EMC rate change). > >>>2) tegra12x_la.c reads MC_EMEM_ARB_MISC0 during a core_initcall to > >>>calculate a value which it then writes to a register that is also > >>>shadowed and that is part of downstream burst registers so that doesn't > >>>do anything either. > >>> > >>>The reason I implemented two ways to specify the MC register area was > >>>that this could be merged before an MC driver and retain > >>>backwards-compatibility after the MC driver arrives. > >>> > >>>If this is not acceptable, we can certainly wait for the MC driver to be > >>>merged first. (Although with the general rate of things, I hope I won't > >>>be back at school at that point..) I assume that this is blocked on the > >>>IOMMU bindings discussion? In that case, there are several options: the > >>>MC driver could have its own tables for each EMC rate or we could just > >>>make the EMC tables global (i.e. not under the EMC node). In any case, > >>>the MC driver would need to implement a function that would just write > >>>these values but be guaranteed to not do anything else, since that could > >>>cause nasty things during the EMC rate change sequence. > >> > >>Having taken another look at the code, I don't think the MC driver could do > >>anything that bad. There are also two other places where the EMC driver > >>needs to read MC registers: Inside the sequence, it reads a register but > >>discards its contents. According to comments, this acts as a memory barrier, > >>probably for the preceding step that writes into MC memory. If the register > >>writes are moved to the MC driver, it could also handle that. In another > >>place it reads the number of RAM modules from a MC register. The MC driver > >>could export this as another function. > > > >Exporting this functionality from the MC driver is the right thing to do > >in my opinion. > > Ok, let's do that then. Do you think I could make a bare-bones MC driver to > support the EMC driver before your MC driver with IOMMU/LA is ready? Can the > MC device tree node be stabilized yet? Of course, if things go well, that > concern might turn out to be unnecessary. Well, at this point this isn't 3.17 material anyway, so there's no need to rush things. I'd prefer to take a patch on top of my proposed MC driver patch in anticipation of merging that for 3.18. But if it turns out that for whatever reason we can't do that, having a separate patch makes it easy to extract the changes into a bare-bones driver. Thierry
On 14/07/14 12:31, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, Jul 14, 2014 at 12:06:32PM +0300, Mikko Perttunen wrote: >> On 14/07/14 11:15, Thierry Reding wrote: >>>> Old Signed by an unknown key >>> >>> On Mon, Jul 14, 2014 at 10:55:51AM +0300, Mikko Perttunen wrote: >>>> On 11/07/14 19:01, Mikko Perttunen wrote: >>>>> On 07/11/2014 05:51 PM, Thierry Reding wrote: >>>>>> On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote: >>>>>>> ... >>>>>> ... >>>>> >>>>> In this case, all the registers that will be written are such that the >>>>> MC driver will never need to write them. They are shadowed registers, >>>>> meaning that all writes are stored and are only effective starting from >>>>> the next time the EMC rate change state machine is activated, so writing >>>>> them from anywhere except than the EMC driver would be pointless. >>>>> >>>>> I can find two users of these registers in downstream: >>>>> 1) mc.c saves and loads them on suspend/restore (I don't know why, that >>>>> shouldn't do anything. They will be overridden anyway during the next >>>>> EMC rate change). >>>>> 2) tegra12x_la.c reads MC_EMEM_ARB_MISC0 during a core_initcall to >>>>> calculate a value which it then writes to a register that is also >>>>> shadowed and that is part of downstream burst registers so that doesn't >>>>> do anything either. >>>>> >>>>> The reason I implemented two ways to specify the MC register area was >>>>> that this could be merged before an MC driver and retain >>>>> backwards-compatibility after the MC driver arrives. >>>>> >>>>> If this is not acceptable, we can certainly wait for the MC driver to be >>>>> merged first. (Although with the general rate of things, I hope I won't >>>>> be back at school at that point..) I assume that this is blocked on the >>>>> IOMMU bindings discussion? In that case, there are several options: the >>>>> MC driver could have its own tables for each EMC rate or we could just >>>>> make the EMC tables global (i.e. not under the EMC node). In any case, >>>>> the MC driver would need to implement a function that would just write >>>>> these values but be guaranteed to not do anything else, since that could >>>>> cause nasty things during the EMC rate change sequence. >>>> >>>> Having taken another look at the code, I don't think the MC driver could do >>>> anything that bad. There are also two other places where the EMC driver >>>> needs to read MC registers: Inside the sequence, it reads a register but >>>> discards its contents. According to comments, this acts as a memory barrier, >>>> probably for the preceding step that writes into MC memory. If the register >>>> writes are moved to the MC driver, it could also handle that. In another >>>> place it reads the number of RAM modules from a MC register. The MC driver >>>> could export this as another function. >>> >>> Exporting this functionality from the MC driver is the right thing to do >>> in my opinion. >> >> Ok, let's do that then. Do you think I could make a bare-bones MC driver to >> support the EMC driver before your MC driver with IOMMU/LA is ready? Can the >> MC device tree node be stabilized yet? Of course, if things go well, that >> concern might turn out to be unnecessary. > > Well, at this point this isn't 3.17 material anyway, so there's no need > to rush things. Very true. > I'd prefer to take a patch on top of my proposed MC > driver patch in anticipation of merging that for 3.18. But if it turns > out that for whatever reason we can't do that, having a separate patch > makes it easy to extract the changes into a bare-bones driver. Yes, this sounds sensible. I'll make such a patch. I suppose having another timings table in the MC node with just the rate and mc-burst-data would separate the concerns best. It occurs to me that we could also write the regs in the pre-rate change notifier, but this would turn the dependency around and would mean that the regs are not written when entering backup rates. The latter shouldn't be a problem but the reversed dependency would, so I guess a custom function is the way to go, and we need to add at least one anyway. The downstream kernel also overwrites most LA registers during EMC rate change without regard for the driver-set values, and we might have to read those values from the device tree too. Upstream can do this in rate change notifiers if needed. I'll look into this a bit more. > > Thierry > > * Unknown Key > * 0x7F3EB3A1 > -- 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 Mon, Jul 14, 2014 at 12:57:26PM +0300, Mikko Perttunen wrote: > On 14/07/14 12:31, Thierry Reding wrote: > >* PGP Signed by an unknown key > > > >On Mon, Jul 14, 2014 at 12:06:32PM +0300, Mikko Perttunen wrote: > >>On 14/07/14 11:15, Thierry Reding wrote: > >>>>Old Signed by an unknown key > >>> > >>>On Mon, Jul 14, 2014 at 10:55:51AM +0300, Mikko Perttunen wrote: > >>>>On 11/07/14 19:01, Mikko Perttunen wrote: > >>>>>On 07/11/2014 05:51 PM, Thierry Reding wrote: > >>>>>>On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote: > >>>>>>>... > >>>>>>... > >>>>> > >>>>>In this case, all the registers that will be written are such that the > >>>>>MC driver will never need to write them. They are shadowed registers, > >>>>>meaning that all writes are stored and are only effective starting from > >>>>>the next time the EMC rate change state machine is activated, so writing > >>>>>them from anywhere except than the EMC driver would be pointless. > >>>>> > >>>>>I can find two users of these registers in downstream: > >>>>>1) mc.c saves and loads them on suspend/restore (I don't know why, that > >>>>>shouldn't do anything. They will be overridden anyway during the next > >>>>>EMC rate change). > >>>>>2) tegra12x_la.c reads MC_EMEM_ARB_MISC0 during a core_initcall to > >>>>>calculate a value which it then writes to a register that is also > >>>>>shadowed and that is part of downstream burst registers so that doesn't > >>>>>do anything either. > >>>>> > >>>>>The reason I implemented two ways to specify the MC register area was > >>>>>that this could be merged before an MC driver and retain > >>>>>backwards-compatibility after the MC driver arrives. > >>>>> > >>>>>If this is not acceptable, we can certainly wait for the MC driver to be > >>>>>merged first. (Although with the general rate of things, I hope I won't > >>>>>be back at school at that point..) I assume that this is blocked on the > >>>>>IOMMU bindings discussion? In that case, there are several options: the > >>>>>MC driver could have its own tables for each EMC rate or we could just > >>>>>make the EMC tables global (i.e. not under the EMC node). In any case, > >>>>>the MC driver would need to implement a function that would just write > >>>>>these values but be guaranteed to not do anything else, since that could > >>>>>cause nasty things during the EMC rate change sequence. > >>>> > >>>>Having taken another look at the code, I don't think the MC driver could do > >>>>anything that bad. There are also two other places where the EMC driver > >>>>needs to read MC registers: Inside the sequence, it reads a register but > >>>>discards its contents. According to comments, this acts as a memory barrier, > >>>>probably for the preceding step that writes into MC memory. If the register > >>>>writes are moved to the MC driver, it could also handle that. In another > >>>>place it reads the number of RAM modules from a MC register. The MC driver > >>>>could export this as another function. > >>> > >>>Exporting this functionality from the MC driver is the right thing to do > >>>in my opinion. > >> > >>Ok, let's do that then. Do you think I could make a bare-bones MC driver to > >>support the EMC driver before your MC driver with IOMMU/LA is ready? Can the > >>MC device tree node be stabilized yet? Of course, if things go well, that > >>concern might turn out to be unnecessary. > > > >Well, at this point this isn't 3.17 material anyway, so there's no need > >to rush things. > > Very true. > > >I'd prefer to take a patch on top of my proposed MC > >driver patch in anticipation of merging that for 3.18. But if it turns > >out that for whatever reason we can't do that, having a separate patch > >makes it easy to extract the changes into a bare-bones driver. > > Yes, this sounds sensible. I'll make such a patch. I suppose having another > timings table in the MC node with just the rate and mc-burst-data would > separate the concerns best. It occurs to me that we could also write the > regs in the pre-rate change notifier, but this would turn the dependency > around and would mean that the regs are not written when entering backup > rates. The latter shouldn't be a problem but the reversed dependency would, > so I guess a custom function is the way to go, and we need to add at least > one anyway. It sounds like maybe moving enough code and data into the MC driver to handle frequency changes would be a good move. From the above it sounds like all the MC driver needs to know is that a frequency change is about to happen and what the new frequency is. In addition to exposing things like number of DRAM banks, etc. > The downstream kernel also overwrites most LA registers during EMC rate > change without regard for the driver-set values, and we might have to read > those values from the device tree too. Upstream can do this in rate change > notifiers if needed. I'll look into this a bit more. As I understand it, the latency allowance should be specified in terms of the maximum amount of time that requests are delayed, so that the proper values for the LA registers can be recomputed on an EMC rate change. Thierry
On 14/07/14 13:29, Thierry Reding wrote: > * PGP Signed by an unknown key > ... >> Yes, this sounds sensible. I'll make such a patch. I suppose having another >> timings table in the MC node with just the rate and mc-burst-data would >> separate the concerns best. It occurs to me that we could also write the >> regs in the pre-rate change notifier, but this would turn the dependency >> around and would mean that the regs are not written when entering backup >> rates. The latter shouldn't be a problem but the reversed dependency would, >> so I guess a custom function is the way to go, and we need to add at least >> one anyway. > > It sounds like maybe moving enough code and data into the MC driver to > handle frequency changes would be a good move. From the above it sounds > like all the MC driver needs to know is that a frequency change is about > to happen and what the new frequency is. > > In addition to exposing things like number of DRAM banks, etc. > Yes, so there are two ways to do this: 1) EMC calls tegra_mc_emem_update(freq) at the correct time 2) MC has an optional clock phandle to the EMC clock and registers a pre-change notifier. Both work, but the dependency is reversed. In both cases, the other driver is also optional. In the first case, the EMC driver can give a warning if the call fails. (As mentioned, if the MC_EMEM updates don't happen, things still work but potentially with a hefty perf loss.) TBH, I haven't yet decided which one is better. If you have an opinion, I'll go with it. >> The downstream kernel also overwrites most LA registers during EMC rate >> change without regard for the driver-set values, and we might have to read >> those values from the device tree too. Upstream can do this in rate change >> notifiers if needed. I'll look into this a bit more. > > As I understand it, the latency allowance should be specified in terms > of the maximum amount of time that requests are delayed, so that the > proper values for the LA registers can be recomputed on an EMC rate > change. That's how I understand it too, but in downstream, the LA register values are hardcoded into EMC tables in platform data/DTS that are just written into the LA registers as-is during rate change. > > Thierry > > * Unknown Key > * 0x7F3EB3A1 > -- 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 Mon, Jul 14, 2014 at 01:54:36PM +0300, Mikko Perttunen wrote: > On 14/07/14 13:29, Thierry Reding wrote: > >* PGP Signed by an unknown key > > ... > >>Yes, this sounds sensible. I'll make such a patch. I suppose having another > >>timings table in the MC node with just the rate and mc-burst-data would > >>separate the concerns best. It occurs to me that we could also write the > >>regs in the pre-rate change notifier, but this would turn the dependency > >>around and would mean that the regs are not written when entering backup > >>rates. The latter shouldn't be a problem but the reversed dependency would, > >>so I guess a custom function is the way to go, and we need to add at least > >>one anyway. > > > >It sounds like maybe moving enough code and data into the MC driver to > >handle frequency changes would be a good move. From the above it sounds > >like all the MC driver needs to know is that a frequency change is about > >to happen and what the new frequency is. > > > >In addition to exposing things like number of DRAM banks, etc. > > > > Yes, so there are two ways to do this: > 1) EMC calls tegra_mc_emem_update(freq) at the correct time > 2) MC has an optional clock phandle to the EMC clock and registers a > pre-change notifier. > > Both work, but the dependency is reversed. In both cases, the other driver > is also optional. In the first case, the EMC driver can give a warning if > the call fails. (As mentioned, if the MC_EMEM updates don't happen, things > still work but potentially with a hefty perf loss.) > TBH, I haven't yet decided which one is better. If you have an opinion, > I'll go with it. I think I prefer 1. Using an explicit call into the MC driver allow us to precisely determine the moment in time when the registers should be updated. The pre-change notifier, as I understand it, doesn't give us that. Also, the notifier doesn't give us a way to determine success or failure of the MC call. > >>The downstream kernel also overwrites most LA registers during EMC rate > >>change without regard for the driver-set values, and we might have to read > >>those values from the device tree too. Upstream can do this in rate change > >>notifiers if needed. I'll look into this a bit more. > > > >As I understand it, the latency allowance should be specified in terms > >of the maximum amount of time that requests are delayed, so that the > >proper values for the LA registers can be recomputed on an EMC rate > >change. > > That's how I understand it too, but in downstream, the LA register values > are hardcoded into EMC tables in platform data/DTS that are just written > into the LA registers as-is during rate change. Hehe, well, we don't want any of that upstream. =) If it can be computed at runtime, then let's compute it. Also, if it's encoded in platform data or DTS, then there's no way it can be adjusted based on use-case. For example if you have a device with two display outputs (an internal panel and HDMI for example) but you never have HDMI plugged in, then there's no reason why you would want to program the latency allowance for the second display controller. If you provide the values in a static frequency/register value table, then you need to account for any possible scenario up front, irrespective of what (if any) HDMI monitor is attached. Thierry
On 14/07/14 14:10, Thierry Reding wrote: > * PGP Signed by an unknown key > > On Mon, Jul 14, 2014 at 01:54:36PM +0300, Mikko Perttunen wrote: >> On 14/07/14 13:29, Thierry Reding wrote: >>>> Old Signed by an unknown key >>> ... >>>> Yes, this sounds sensible. I'll make such a patch. I suppose having another >>>> timings table in the MC node with just the rate and mc-burst-data would >>>> separate the concerns best. It occurs to me that we could also write the >>>> regs in the pre-rate change notifier, but this would turn the dependency >>>> around and would mean that the regs are not written when entering backup >>>> rates. The latter shouldn't be a problem but the reversed dependency would, >>>> so I guess a custom function is the way to go, and we need to add at least >>>> one anyway. >>> >>> It sounds like maybe moving enough code and data into the MC driver to >>> handle frequency changes would be a good move. From the above it sounds >>> like all the MC driver needs to know is that a frequency change is about >>> to happen and what the new frequency is. >>> >>> In addition to exposing things like number of DRAM banks, etc. >>> >> >> Yes, so there are two ways to do this: >> 1) EMC calls tegra_mc_emem_update(freq) at the correct time >> 2) MC has an optional clock phandle to the EMC clock and registers a >> pre-change notifier. >> >> Both work, but the dependency is reversed. In both cases, the other driver >> is also optional. In the first case, the EMC driver can give a warning if >> the call fails. (As mentioned, if the MC_EMEM updates don't happen, things >> still work but potentially with a hefty perf loss.) >> TBH, I haven't yet decided which one is better. If you have an opinion, >> I'll go with it. > > I think I prefer 1. Using an explicit call into the MC driver allow us > to precisely determine the moment in time when the registers should be > updated. The pre-change notifier, as I understand it, doesn't give us > that. Also, the notifier doesn't give us a way to determine success or > failure of the MC call. Indeed. I'll go with this. > >>>> The downstream kernel also overwrites most LA registers during EMC rate >>>> change without regard for the driver-set values, and we might have to read >>>> those values from the device tree too. Upstream can do this in rate change >>>> notifiers if needed. I'll look into this a bit more. >>> >>> As I understand it, the latency allowance should be specified in terms >>> of the maximum amount of time that requests are delayed, so that the >>> proper values for the LA registers can be recomputed on an EMC rate >>> change. >> >> That's how I understand it too, but in downstream, the LA register values >> are hardcoded into EMC tables in platform data/DTS that are just written >> into the LA registers as-is during rate change. > > Hehe, well, we don't want any of that upstream. =) If it can be computed > at runtime, then let's compute it. Also, if it's encoded in platform > data or DTS, then there's no way it can be adjusted based on use-case. > For example if you have a device with two display outputs (an internal > panel and HDMI for example) but you never have HDMI plugged in, then > there's no reason why you would want to program the latency allowance > for the second display controller. If you provide the values in a static > frequency/register value table, then you need to account for any > possible scenario up front, irrespective of what (if any) HDMI monitor > is attached. Yeah, I guess the values in downstream must be designed for the worst case :P the strange thing is that downstream also has an API for drivers to specify their requirements. I guess the clients that have hardcoded values and that use the API don't overlap. But I definitely agree that on upstream we can have something nicer. > > Thierry > > * Unknown Key > * 0x7F3EB3A1 > -- 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 07/11/2014 10:43 AM, Andrew Bresticker wrote: > On Fri, Jul 11, 2014 at 7:18 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote: >> Add binding documentation for the nvidia,tegra124-emc device tree >> node. > >> diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt > >> +Required properties : >> +- compatible : "nvidia,tegra124-emc". >> +- reg : Should contain 1 or 2 entries: >> + - EMC register set >> + - MC register set : Required only if no node with >> + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set >> + is first read from the MC node. If it doesn't exist, it is read >> + from this property. >> +- timings : Should contain 1 entry for each supported clock rate. >> + Entries should be named "timing@n" where n is a 0-based increasing >> + number. The timings must be listed in rate-ascending order. > > There are upcoming boards which support multiple DRAM configurations > and require a separate set of timings for each configuration. Could > we instead have multiple sets of timings with the proper one selected > at runtime by RAM code, as reported by PMC_STRAPPING_OPT_A_0? > Something like: > > emc { > emc-table@0 { > nvidia,ram-code = <0>; > timing@0 { > ... > }; > ... > }; Until recently, there was a binding for Tegra20 EMC in mainline. We should make sure the Tegra124 driver (or rather binding...) is at least as feature-ful as that was. Furthermore, I thought that with some boards, there were more RAM options that there were available RAM code strap bits. I assume that in mainline, we'll simply have different base DT files for the different sets of configurations? Or, will we want to add another level to the DT to represent different sets of RAM code values? I'm not sure what data source the bootloader uses to determine which set of RAM configuration to use when there aren't enough entries in the BCT for the boot ROM to do this automatically, and whether we have any way to get that value into the kernel, so it could use it for the extra DT level lookup? -- 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 07/11/2014 08:18 AM, Mikko Perttunen wrote: > Add binding documentation for the nvidia,tegra124-emc device tree > node. > diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt > +Required properties : > +- compatible : "nvidia,tegra124-emc". > +- reg : Should contain 1 or 2 entries: > + - EMC register set > + - MC register set : Required only if no node with > + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set > + is first read from the MC node. If it doesn't exist, it is read > + from this property. I echo what Thierry said about the MC register sharing. > +- timings : Should contain 1 entry for each supported clock rate. s/entry/node/ I think. > + Entries should be named "timing@n" where n is a 0-based increasing > + number. The timings must be listed in rate-ascending order. That implies that #address-cells and #size-cells are required too. > +Required properties for timings : ... and a reg property is needed here. > +- clock-frequency : Should contain the memory clock rate. > +- nvidia,parent-clock-frequency : Should contain the rate of the EMC > + clock's parent clock. > +- clocks : Must contain an entry for each entry in clock-names. > + See ../clocks/clock-bindings.txt for details. > +- clock-names : Must include the following entries: > + - emc-parent : EMC's parent clock. Shouldn't clocks/clock-names be part of the main node, not the per-timing node? > +- The following properties contain EMC timing characterization values: > + - nvidia,emc-zcal-cnt-long > + - nvidia,emc-auto-cal-interval > + - nvidia,emc-ctt-term-ctrl > + - nvidia,emc-cfg > + - nvidia,emc-cfg-2 > + - nvidia,emc-sel-dpd-ctrl > + - nvidia,emc-cfg-dig-dll > + - nvidia,emc-bgbias-ctl0 > + - nvidia,emc-auto-cal-config > + - nvidia,emc-auto-cal-config2 > + - nvidia,emc-auto-cal-config3 > + - nvidia,emc-mode-reset > + - nvidia,emc-mode-1 > + - nvidia,emc-mode-2 > + - nvidia,emc-mode-4 > +- nvidia,emc-burst-data : EMC timing characterization data written to > + EMC registers. > +- nvidia,mc-burst-data : EMC timing characterization data written to > + MC registers. > -- 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 Mon, Jul 21, 2014 at 2:28 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 07/11/2014 10:43 AM, Andrew Bresticker wrote: >> On Fri, Jul 11, 2014 at 7:18 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote: >>> Add binding documentation for the nvidia,tegra124-emc device tree >>> node. >> >>> diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt >> >>> +Required properties : >>> +- compatible : "nvidia,tegra124-emc". >>> +- reg : Should contain 1 or 2 entries: >>> + - EMC register set >>> + - MC register set : Required only if no node with >>> + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set >>> + is first read from the MC node. If it doesn't exist, it is read >>> + from this property. >>> +- timings : Should contain 1 entry for each supported clock rate. >>> + Entries should be named "timing@n" where n is a 0-based increasing >>> + number. The timings must be listed in rate-ascending order. >> >> There are upcoming boards which support multiple DRAM configurations >> and require a separate set of timings for each configuration. Could >> we instead have multiple sets of timings with the proper one selected >> at runtime by RAM code, as reported by PMC_STRAPPING_OPT_A_0? >> Something like: >> >> emc { >> emc-table@0 { >> nvidia,ram-code = <0>; >> timing@0 { >> ... >> }; >> ... >> }; > > Until recently, there was a binding for Tegra20 EMC in mainline. We > should make sure the Tegra124 driver (or rather binding...) is at least > as feature-ful as that was. > > Furthermore, I thought that with some boards, there were more RAM > options that there were available RAM code strap bits. I assume that in > mainline, we'll simply have different base DT files for the different > sets of configurations? Or, will we want to add another level to the DT > to represent different sets of RAM code values? I'm not sure what data > source the bootloader uses to determine which set of RAM configuration > to use when there aren't enough entries in the BCT for the boot ROM to > do this automatically, and whether we have any way to get that value > into the kernel, so it could use it for the extra DT level lookup? For the ChromeOS boards at least we are neither limited by the number of strapping bits (4) nor the number of BCT entries supported by the boot ROM (since coreboot does not rely on the boot ROM for SDRAM initialization), so having a single set of SDRAM configurations for each board, indexed by APBDEV_PMC_STRAPPING_OPT_A_0[7:4] works just fine. Not sure how boards which do use the boot ROM for SDRAM initialization handle the BCT limitation. I believe we considered using more board ID strapping bits in order to map the RAM codes to sets of configurations before switching to having coreboot do SDRAM initialization. -- 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 07/21/2014 04:52 PM, Andrew Bresticker wrote: > On Mon, Jul 21, 2014 at 2:28 PM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 07/11/2014 10:43 AM, Andrew Bresticker wrote: >>> On Fri, Jul 11, 2014 at 7:18 AM, Mikko Perttunen <mperttunen@nvidia.com> wrote: >>>> Add binding documentation for the nvidia,tegra124-emc device tree >>>> node. >>> >>>> diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt >>> >>>> +Required properties : >>>> +- compatible : "nvidia,tegra124-emc". >>>> +- reg : Should contain 1 or 2 entries: >>>> + - EMC register set >>>> + - MC register set : Required only if no node with >>>> + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set >>>> + is first read from the MC node. If it doesn't exist, it is read >>>> + from this property. >>>> +- timings : Should contain 1 entry for each supported clock rate. >>>> + Entries should be named "timing@n" where n is a 0-based increasing >>>> + number. The timings must be listed in rate-ascending order. >>> >>> There are upcoming boards which support multiple DRAM configurations >>> and require a separate set of timings for each configuration. Could >>> we instead have multiple sets of timings with the proper one selected >>> at runtime by RAM code, as reported by PMC_STRAPPING_OPT_A_0? >>> Something like: >>> >>> emc { >>> emc-table@0 { >>> nvidia,ram-code = <0>; >>> timing@0 { >>> ... >>> }; >>> ... >>> }; >> >> Until recently, there was a binding for Tegra20 EMC in mainline. We >> should make sure the Tegra124 driver (or rather binding...) is at least >> as feature-ful as that was. >> >> Furthermore, I thought that with some boards, there were more RAM >> options that there were available RAM code strap bits. I assume that in >> mainline, we'll simply have different base DT files for the different >> sets of configurations? Or, will we want to add another level to the DT >> to represent different sets of RAM code values? I'm not sure what data >> source the bootloader uses to determine which set of RAM configuration >> to use when there aren't enough entries in the BCT for the boot ROM to >> do this automatically, and whether we have any way to get that value >> into the kernel, so it could use it for the extra DT level lookup? > > For the ChromeOS boards at least we are neither limited by the number > of strapping bits (4) nor the number of BCT entries supported by the > boot ROM (since coreboot does not rely on the boot ROM for SDRAM > initialization), so having a single set of SDRAM configurations for > each board, indexed by APBDEV_PMC_STRAPPING_OPT_A_0[7:4] works just > fine. Does the bootloader adjust the DT that's passed to the kernel so that only the relevant single set of EMC timings is contained in the DT? On a system where the boot ROM initializes RAM, and where the HW design might have multiple SDRAM configuration, here's what usually happens: a) The BCT contains N sets of SDRAM configurations. b) The boot ROM reads the SDRAM strapping bits, and uses them to pick the correct SDRAM configuration from the N sets in the BCT. c) The kernel DT has N sets of SDRAM configurations. d) The kernel reads the SDRAM strapping bits, and uses them to pick the correct SDRAM configuration from the N sets in the DT. On the ChromeOS boards (so (a) and (b) above are irrelevant) where N is too large to fit into APBDEV_PMC_STRAPPING_OPT_A_0[7:4], (c) and (d) won't work. I assume the kernel DT therefore must be adjusted to only contain the single SDRAM configuration that is relevant for the current HW? (isn't STRAPPING_OPT_A split into 2 2-bit fields; 2 bits for SDRAM index and 2 bits for boot flash index, so max N is quite small?) -- 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, Jul 22, 2014 at 9:45 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > > Does the bootloader adjust the DT that's passed to the kernel so that > only the relevant single set of EMC timings is contained in the DT? No, the DT contains all possible EMC timings for that board. > On a system where the boot ROM initializes RAM, and where the HW design > might have multiple SDRAM configuration, here's what usually happens: > > a) The BCT contains N sets of SDRAM configurations. > > b) The boot ROM reads the SDRAM strapping bits, and uses them to pick > the correct SDRAM configuration from the N sets in the BCT. > > c) The kernel DT has N sets of SDRAM configurations. > > d) The kernel reads the SDRAM strapping bits, and uses them to pick the > correct SDRAM configuration from the N sets in the DT. > > On the ChromeOS boards (so (a) and (b) above are irrelevant) where N is > too large to fit into APBDEV_PMC_STRAPPING_OPT_A_0[7:4], (c) and (d) > won't work. I assume the kernel DT therefore must be adjusted to only > contain the single SDRAM configuration that is relevant for the current HW? > > (isn't STRAPPING_OPT_A split into 2 2-bit fields; 2 bits for SDRAM index > and 2 bits for boot flash index, so max N is quite small?) Right, there are normally only 2 SDRAM strapping bits available. ChromeOS gets around this by having 4 identical boot device entries in the BCT, so all possible values of STRAPPING_OPT_A[7:6] map to the same boot device. This allows us to use all 4 strapping bits in coreboot to pick the SDRAM configuration. -- 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 07/22/2014 11:22 AM, Andrew Bresticker wrote: > On Tue, Jul 22, 2014 at 9:45 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: >> >> Does the bootloader adjust the DT that's passed to the kernel so that >> only the relevant single set of EMC timings is contained in the DT? > > No, the DT contains all possible EMC timings for that board. > >> On a system where the boot ROM initializes RAM, and where the HW design >> might have multiple SDRAM configuration, here's what usually happens: >> >> a) The BCT contains N sets of SDRAM configurations. >> >> b) The boot ROM reads the SDRAM strapping bits, and uses them to pick >> the correct SDRAM configuration from the N sets in the BCT. >> >> c) The kernel DT has N sets of SDRAM configurations. >> >> d) The kernel reads the SDRAM strapping bits, and uses them to pick the >> correct SDRAM configuration from the N sets in the DT. >> >> On the ChromeOS boards (so (a) and (b) above are irrelevant) where N is >> too large to fit into APBDEV_PMC_STRAPPING_OPT_A_0[7:4], (c) and (d) >> won't work. I assume the kernel DT therefore must be adjusted to only >> contain the single SDRAM configuration that is relevant for the current HW? >> >> (isn't STRAPPING_OPT_A split into 2 2-bit fields; 2 bits for SDRAM index >> and 2 bits for boot flash index, so max N is quite small?) > > Right, there are normally only 2 SDRAM strapping bits available. > ChromeOS gets around this by having 4 identical boot device entries in > the BCT, so all possible values of STRAPPING_OPT_A[7:6] map to the > same boot device. This allows us to use all 4 strapping bits in > coreboot to pick the SDRAM configuration. OK, that explains how it works. But that means that when the kernel reads the strapping options, it will have to know if it uses just 2 bits (standard) or all 4 bits (non-standard) to index into the DT's array of SDRAM configurations. We'll need a DT property to represent that. -- 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
Looks like the TRM doesn't document this either. I'll add an option
("nvidia,short-ram-code" ?) for the next version.
On 22/07/14 20:34, Stephen Warren wrote:
> On 07/22/2014 11:22 AM, Andrew Bresticker wrote:
>> On Tue, Jul 22, 2014 at 9:45 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>
>>> Does the bootloader adjust the DT that's passed to the kernel so that
>>> only the relevant single set of EMC timings is contained in the DT?
>>
>> No, the DT contains all possible EMC timings for that board.
>>
>>> On a system where the boot ROM initializes RAM, and where the HW design
>>> might have multiple SDRAM configuration, here's what usually happens:
>>>
>>> a) The BCT contains N sets of SDRAM configurations.
>>>
>>> b) The boot ROM reads the SDRAM strapping bits, and uses them to pick
>>> the correct SDRAM configuration from the N sets in the BCT.
>>>
>>> c) The kernel DT has N sets of SDRAM configurations.
>>>
>>> d) The kernel reads the SDRAM strapping bits, and uses them to pick the
>>> correct SDRAM configuration from the N sets in the DT.
>>>
>>> On the ChromeOS boards (so (a) and (b) above are irrelevant) where N is
>>> too large to fit into APBDEV_PMC_STRAPPING_OPT_A_0[7:4], (c) and (d)
>>> won't work. I assume the kernel DT therefore must be adjusted to only
>>> contain the single SDRAM configuration that is relevant for the current HW?
>>>
>>> (isn't STRAPPING_OPT_A split into 2 2-bit fields; 2 bits for SDRAM index
>>> and 2 bits for boot flash index, so max N is quite small?)
>>
>> Right, there are normally only 2 SDRAM strapping bits available.
>> ChromeOS gets around this by having 4 identical boot device entries in
>> the BCT, so all possible values of STRAPPING_OPT_A[7:6] map to the
>> same boot device. This allows us to use all 4 strapping bits in
>> coreboot to pick the SDRAM configuration.
>
> OK, that explains how it works.
>
> But that means that when the kernel reads the strapping options, it will
> have to know if it uses just 2 bits (standard) or all 4 bits
> (non-standard) to index into the DT's array of SDRAM configurations.
> We'll need a DT property to represent that.
> --
> 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
>
--
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 07/29/2014 02:30 AM, Mikko Perttunen wrote: > Looks like the TRM doesn't document this either. I'll add an option > ("nvidia,short-ram-code" ?) for the next version. Using the 2-bit RAM code field as the RAM code is normal operation, so I wouldn't call this "short". Using the 2-bit boot device code field as extra RAM code bits is non-standard. I would suggest nvidia,use-4-bit-ram-code or nvidia,use-boot-device-code-as-ram-code-msbs(!) as the property. I see that the TRM implies the whole 4-bit field is RAM code, rather than there being 2 separate 2-bit fields for RAM code and boot device code. Can you please file a bug against the TRM to document this correctly? (The details of which bits are which are visible on the Jetson TK1 schematics for example). > On 22/07/14 20:34, Stephen Warren wrote: >> On 07/22/2014 11:22 AM, Andrew Bresticker wrote: >>> On Tue, Jul 22, 2014 at 9:45 AM, Stephen Warren >>> <swarren@wwwdotorg.org> wrote: >>>> >>>> Does the bootloader adjust the DT that's passed to the kernel so that >>>> only the relevant single set of EMC timings is contained in the DT? >>> >>> No, the DT contains all possible EMC timings for that board. >>> >>>> On a system where the boot ROM initializes RAM, and where the HW design >>>> might have multiple SDRAM configuration, here's what usually happens: >>>> >>>> a) The BCT contains N sets of SDRAM configurations. >>>> >>>> b) The boot ROM reads the SDRAM strapping bits, and uses them to pick >>>> the correct SDRAM configuration from the N sets in the BCT. >>>> >>>> c) The kernel DT has N sets of SDRAM configurations. >>>> >>>> d) The kernel reads the SDRAM strapping bits, and uses them to pick the >>>> correct SDRAM configuration from the N sets in the DT. >>>> >>>> On the ChromeOS boards (so (a) and (b) above are irrelevant) where N is >>>> too large to fit into APBDEV_PMC_STRAPPING_OPT_A_0[7:4], (c) and (d) >>>> won't work. I assume the kernel DT therefore must be adjusted to only >>>> contain the single SDRAM configuration that is relevant for the >>>> current HW? >>>> >>>> (isn't STRAPPING_OPT_A split into 2 2-bit fields; 2 bits for SDRAM >>>> index >>>> and 2 bits for boot flash index, so max N is quite small?) >>> >>> Right, there are normally only 2 SDRAM strapping bits available. >>> ChromeOS gets around this by having 4 identical boot device entries in >>> the BCT, so all possible values of STRAPPING_OPT_A[7:6] map to the >>> same boot device. This allows us to use all 4 strapping bits in >>> coreboot to pick the SDRAM configuration. >> >> OK, that explains how it works. >> >> But that means that when the kernel reads the strapping options, it will >> have to know if it uses just 2 bits (standard) or all 4 bits >> (non-standard) to index into the DT's array of SDRAM configurations. >> We'll need a DT property to represent that. >> -- >> 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 >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- 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 29/07/14 18:49, Stephen Warren wrote: > On 07/29/2014 02:30 AM, Mikko Perttunen wrote: >> Looks like the TRM doesn't document this either. I'll add an option >> ("nvidia,short-ram-code" ?) for the next version. > > Using the 2-bit RAM code field as the RAM code is normal operation, so I > wouldn't call this "short". > > Using the 2-bit boot device code field as extra RAM code bits is > non-standard. > > I would suggest nvidia,use-4-bit-ram-code or > nvidia,use-boot-device-code-as-ram-code-msbs(!) as the property. Sure. > > I see that the TRM implies the whole 4-bit field is RAM code, rather > than there being 2 separate 2-bit fields for RAM code and boot device > code. Can you please file a bug against the TRM to document this > correctly? (The details of which bits are which are visible on the > Jetson TK1 schematics for example). Yes, I'll file a bug. - Mikko -- 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 31/07/14 13:48, Mikko Perttunen wrote: >> >> I see that the TRM implies the whole 4-bit field is RAM code, rather >> than there being 2 separate 2-bit fields for RAM code and boot device >> code. Can you please file a bug against the TRM to document this >> correctly? (The details of which bits are which are visible on the >> Jetson TK1 schematics for example). > > Yes, I'll file a bug. On a closer look, the downstream kernel has been recently updated to consider the whole 4 bits the ram code. The relevant bug also has a comment mentioning that starting from T124, the whole 4 bits is considered the RAM code. > > - Mikko > -- > 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 > -- 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 07/31/2014 05:05 AM, Mikko Perttunen wrote: > On 31/07/14 13:48, Mikko Perttunen wrote: >>> >>> I see that the TRM implies the whole 4-bit field is RAM code, rather >>> than there being 2 separate 2-bit fields for RAM code and boot device >>> code. Can you please file a bug against the TRM to document this >>> correctly? (The details of which bits are which are visible on the >>> Jetson TK1 schematics for example). >> >> Yes, I'll file a bug. > > On a closer look, the downstream kernel has been recently updated to > consider the whole 4 bits the ram code. The relevant bug also has a > comment mentioning that starting from T124, the whole 4 bits is > considered the RAM code. That's odd. Given the structure of the BCT hasn't change, I suspect that's a documentation bug that's propagated elsewhere. Can you send me the bug number internally, and I'll take a look? Thanks. -- 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/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt new file mode 100644 index 0000000..2dde17e --- /dev/null +++ b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt @@ -0,0 +1,42 @@ +Tegra124 SoC EMC controller + +Required properties : +- compatible : "nvidia,tegra124-emc". +- reg : Should contain 1 or 2 entries: + - EMC register set + - MC register set : Required only if no node with + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set + is first read from the MC node. If it doesn't exist, it is read + from this property. +- timings : Should contain 1 entry for each supported clock rate. + Entries should be named "timing@n" where n is a 0-based increasing + number. The timings must be listed in rate-ascending order. + +Required properties for timings : +- clock-frequency : Should contain the memory clock rate. +- nvidia,parent-clock-frequency : Should contain the rate of the EMC + clock's parent clock. +- clocks : Must contain an entry for each entry in clock-names. + See ../clocks/clock-bindings.txt for details. +- clock-names : Must include the following entries: + - emc-parent : EMC's parent clock. +- The following properties contain EMC timing characterization values: + - nvidia,emc-zcal-cnt-long + - nvidia,emc-auto-cal-interval + - nvidia,emc-ctt-term-ctrl + - nvidia,emc-cfg + - nvidia,emc-cfg-2 + - nvidia,emc-sel-dpd-ctrl + - nvidia,emc-cfg-dig-dll + - nvidia,emc-bgbias-ctl0 + - nvidia,emc-auto-cal-config + - nvidia,emc-auto-cal-config2 + - nvidia,emc-auto-cal-config3 + - nvidia,emc-mode-reset + - nvidia,emc-mode-1 + - nvidia,emc-mode-2 + - nvidia,emc-mode-4 +- nvidia,emc-burst-data : EMC timing characterization data written to + EMC registers. +- nvidia,mc-burst-data : EMC timing characterization data written to + MC registers.
Add binding documentation for the nvidia,tegra124-emc device tree node. Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- .../bindings/memory-controllers/tegra-emc.txt | 42 ++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt