| Message ID | 1461099580-3866-39-git-send-email-swarren@wwwdotorg.org |
|---|---|
| State | Rejected |
| Delegated to: | Tom Warren |
| Headers | show |
Hi Stephen, On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> wrote: > From: Stephen Warren <swarren@nvidia.com> > > U-Boot is compiled for a single board, which in turn uses a specific SoC. > There's no need to make runtime decisions based on SoC ID. While there's > certainly an argument for making the code support different SoCs at > run-time, the Tegra code is so far from that possible ideal that the > existing runtime code is an anomaly. If this changes in the future, all > runtime decisions should likely be based on DT anyway. > > Signed-off-by: Stephen Warren <swarren@nvidia.com> > --- > arch/arm/mach-tegra/ap.c | 106 ++++++++++----------------------- > arch/arm/mach-tegra/cache.c | 20 +++---- > arch/arm/mach-tegra/cpu.c | 16 ++--- > arch/arm/mach-tegra/cpu.h | 6 -- > arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- > 5 files changed, 51 insertions(+), 117 deletions(-) What exactly is missing to prevent multi-arch support? Shouldn't we head towards that rather than making it harder? Regards, Simon
On 04/23/2016 11:14 AM, Simon Glass wrote: > Hi Stephen, > > On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> wrote: >> From: Stephen Warren <swarren@nvidia.com> >> >> U-Boot is compiled for a single board, which in turn uses a specific SoC. >> There's no need to make runtime decisions based on SoC ID. While there's >> certainly an argument for making the code support different SoCs at >> run-time, the Tegra code is so far from that possible ideal that the >> existing runtime code is an anomaly. If this changes in the future, all >> runtime decisions should likely be based on DT anyway. >> >> Signed-off-by: Stephen Warren <swarren@nvidia.com> >> --- >> arch/arm/mach-tegra/ap.c | 106 ++++++++++----------------------- >> arch/arm/mach-tegra/cache.c | 20 +++---- >> arch/arm/mach-tegra/cpu.c | 16 ++--- >> arch/arm/mach-tegra/cpu.h | 6 -- >> arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- >> 5 files changed, 51 insertions(+), 117 deletions(-) > > What exactly is missing to prevent multi-arch support? In a word: everything:-) Pretty much all decisions in core architecture code, core Tegra code, drivers, and even board files are currently made at compile time. For example, consider drivers where the register layouts are different between different SoCs; not just new fields added, but existing fields moved to different offsets. Right now, we handle this by changing the register struct definition at compile time. To support multiple chips, we'd have to either (a) link in n copies of the driver, one per register layout, or (b) rework the driver to use #defines and runtime calculations for register offsets, like the Linux kernel drivers do. Tegra USB is one example. The pinmux and clock drivers have a significantly different sets of pins/clocks/resets/... per SoC, and enums/tables describing those sets are currently configured at compile time. Some PMIC constants (e.g. vdd_cpu voltage) are configured at compile-time, and even differ per board. > Shouldn't we head towards that rather than making it harder? I don't see any need for that, no. U-Boot is built for a specific board (or in some cases a set of extremely closely related set of boards, such as the RPI A/B/A+/B+). There's no need to determine almost anything at run-time since almost all information is known at compile time, with exceptions such as standardized enumerable buses such as USB, PCIe. If we support multiple HW in a single binary, it gets bloated with code that simply isn't going to be used, since all the extra code is either for a platform that the build won't be installed on (e.g. clock/pinmux tables), or is overhead to add runtime detection of which block of code to use, which simply isn't needed in the current model. In my opinion, firmware/bootloaders run on a single specific board, whereas full-featured operating systems support multiple systems. As an aside, I've wondered whether U-Boot should be split into multiple parts; one HW-specific binary providing various drivers (e.g. via DM-related APIs?) and the other containing just high-level user-interface code such as the shell, high-level USB/... protocols, which would only call into those APIs. Still, I don't think we're anywhere close to that, and I'm not aware that it's a goal of the project at the moment.
Hi Stephen, On 25 April 2016 at 13:25, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/23/2016 11:14 AM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> From: Stephen Warren <swarren@nvidia.com> >>> >>> U-Boot is compiled for a single board, which in turn uses a specific SoC. >>> There's no need to make runtime decisions based on SoC ID. While there's >>> certainly an argument for making the code support different SoCs at >>> run-time, the Tegra code is so far from that possible ideal that the >>> existing runtime code is an anomaly. If this changes in the future, all >>> runtime decisions should likely be based on DT anyway. >>> >>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>> --- >>> arch/arm/mach-tegra/ap.c | 106 >>> ++++++++++----------------------- >>> arch/arm/mach-tegra/cache.c | 20 +++---- >>> arch/arm/mach-tegra/cpu.c | 16 ++--- >>> arch/arm/mach-tegra/cpu.h | 6 -- >>> arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- >>> 5 files changed, 51 insertions(+), 117 deletions(-) >> >> >> What exactly is missing to prevent multi-arch support? > > > In a word: everything:-) > > Pretty much all decisions in core architecture code, core Tegra code, > drivers, and even board files are currently made at compile time. For > example, consider drivers where the register layouts are different between > different SoCs; not just new fields added, but existing fields moved to > different offsets. Right now, we handle this by changing the register struct > definition at compile time. To support multiple chips, we'd have to either > (a) link in n copies of the driver, one per register layout, or (b) rework > the driver to use #defines and runtime calculations for register offsets, > like the Linux kernel drivers do. Tegra USB is one example. The pinmux and > clock drivers have a significantly different sets of pins/clocks/resets/... > per SoC, and enums/tables describing those sets are currently configured at > compile time. Some PMIC constants (e.g. vdd_cpu voltage) are configured at > compile-time, and even differ per board. I wonder how far we would get by converting clock, pinctrl, reset to driver model drivers? > >> Shouldn't we head towards that rather than making it harder? > > > I don't see any need for that, no. > > U-Boot is built for a specific board (or in some cases a set of extremely > closely related set of boards, such as the RPI A/B/A+/B+). There's no need > to determine almost anything at run-time since almost all information is > known at compile time, with exceptions such as standardized enumerable buses > such as USB, PCIe. If we support multiple HW in a single binary, it gets > bloated with code that simply isn't going to be used, since all the extra > code is either for a platform that the build won't be installed on (e.g. > clock/pinmux tables), or is overhead to add runtime detection of which block > of code to use, which simply isn't needed in the current model. It's not so much that. Presumably a build for a particular board would not include support for and SoC it doesn't have. But it is still useful to build the code. For example it would be nice to have an overall Tegra build that enables all SoCs to avoid building every board. So it is a serious question. I suspect the main impediment may be moving the clock and other core stuff to driver model. > > In my opinion, firmware/bootloaders run on a single specific board, whereas > full-featured operating systems support multiple systems. Except when the boards are pretty similar. Also, doesn't barebox have only one build for Tegra? > > As an aside, I've wondered whether U-Boot should be split into multiple > parts; one HW-specific binary providing various drivers (e.g. via DM-related > APIs?) and the other containing just high-level user-interface code such as > the shell, high-level USB/... protocols, which would only call into those > APIs. Still, I don't think we're anywhere close to that, and I'm not aware > that it's a goal of the project at the moment. Well it gets built as one binary, but there's a pretty clear separation in the code, at least with driver model. What's the purpose of this? Regards, Simon
On 04/27/2016 08:50 AM, Simon Glass wrote: > Hi Stephen, > > On 25 April 2016 at 13:25, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/23/2016 11:14 AM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> From: Stephen Warren <swarren@nvidia.com> >>>> >>>> U-Boot is compiled for a single board, which in turn uses a specific SoC. >>>> There's no need to make runtime decisions based on SoC ID. While there's >>>> certainly an argument for making the code support different SoCs at >>>> run-time, the Tegra code is so far from that possible ideal that the >>>> existing runtime code is an anomaly. If this changes in the future, all >>>> runtime decisions should likely be based on DT anyway. >>>> >>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>> --- >>>> arch/arm/mach-tegra/ap.c | 106 >>>> ++++++++++----------------------- >>>> arch/arm/mach-tegra/cache.c | 20 +++---- >>>> arch/arm/mach-tegra/cpu.c | 16 ++--- >>>> arch/arm/mach-tegra/cpu.h | 6 -- >>>> arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- >>>> 5 files changed, 51 insertions(+), 117 deletions(-) >>> >>> >>> What exactly is missing to prevent multi-arch support? >> >> In a word: everything:-) >> >> Pretty much all decisions in core architecture code, core Tegra code, >> drivers, and even board files are currently made at compile time. For >> example, consider drivers where the register layouts are different between >> different SoCs; not just new fields added, but existing fields moved to >> different offsets. Right now, we handle this by changing the register struct >> definition at compile time. To support multiple chips, we'd have to either >> (a) link in n copies of the driver, one per register layout, or (b) rework >> the driver to use #defines and runtime calculations for register offsets, >> like the Linux kernel drivers do. Tegra USB is one example. The pinmux and >> clock drivers have a significantly different sets of pins/clocks/resets/... >> per SoC, and enums/tables describing those sets are currently configured at >> compile time. Some PMIC constants (e.g. vdd_cpu voltage) are configured at >> compile-time, and even differ per board. > > I wonder how far we would get by converting clock, pinctrl, reset to > driver model drivers? Well, I expect we'll find out soon. The next SoC has radically different clock/reset mechanisms, so we'll need to switch to standardized APIs for clock/reset on Tegra to isolate drivers from those differences, and I imagine that conversion would also involve conversion to DM since any standard APIs probably assume use of DM. I haven't investigated this in detail yet though. >>> Shouldn't we head towards that rather than making it harder? >> >> I don't see any need for that, no. >> >> U-Boot is built for a specific board (or in some cases a set of extremely >> closely related set of boards, such as the RPI A/B/A+/B+). There's no need >> to determine almost anything at run-time since almost all information is >> known at compile time, with exceptions such as standardized enumerable buses >> such as USB, PCIe. If we support multiple HW in a single binary, it gets >> bloated with code that simply isn't going to be used, since all the extra >> code is either for a platform that the build won't be installed on (e.g. >> clock/pinmux tables), or is overhead to add runtime detection of which block >> of code to use, which simply isn't needed in the current model. > > It's not so much that. Presumably a build for a particular board would > not include support for and SoC it doesn't have. But it is still > useful to build the code. For example it would be nice to have an > overall Tegra build that enables all SoCs to avoid building every > board. > > So it is a serious question. I suspect the main impediment may be > moving the clock and other core stuff to driver model. Yes, everything is a bit too tightly coupled at the moment, and in many cases each SoC-specific implementation exposes the same global symbols which clients use. DM or similar conversions may well solve a lot of this. >> In my opinion, firmware/bootloaders run on a single specific board, whereas >> full-featured operating systems support multiple systems. > > Except when the boards are pretty similar. Also, doesn't barebox have > only one build for Tegra? I haven't looked at Barebox much. IIRC it only supports Tegra20 and not later SoCs which could simplify things. Besides, I'm not arguing that it's impossible to make a unified binary, simply that I don't see any need to do so, except perhaps your compile-coverage suggestion. >> As an aside, I've wondered whether U-Boot should be split into multiple >> parts; one HW-specific binary providing various drivers (e.g. via DM-related >> APIs?) and the other containing just high-level user-interface code such as >> the shell, high-level USB/... protocols, which would only call into those >> APIs. Still, I don't think we're anywhere close to that, and I'm not aware >> that it's a goal of the project at the moment. > > Well it gets built as one binary, but there's a pretty clear > separation in the code, at least with driver model. What's the purpose > of this? It would allow the HW-agnostic portion to be compiled once (or once for a CPU ISA) and re-used with any of the HW-/board-specific "driver" blobs. It'd get use to "single binary" for the generic stuff, but without requiring the for HW-specific code. Perhaps the generic portion could even run on top of other driver stacks if they implemented the API it needed! However, this does ignore potential feature differences in the common binary, e.g. someone might want dfu/ums commands, but someone else might not need them and hence consider them bloat. Still, those configurations would be differentiated by feature more than HW, so it might still be useful.
Hi Stephen, On 27 April 2016 at 10:13, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/27/2016 08:50 AM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 25 April 2016 at 13:25, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> On 04/23/2016 11:14 AM, Simon Glass wrote: >>>> >>>> >>>> Hi Stephen, >>>> >>>> On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> >>>>> >>>>> From: Stephen Warren <swarren@nvidia.com> >>>>> >>>>> U-Boot is compiled for a single board, which in turn uses a specific >>>>> SoC. >>>>> There's no need to make runtime decisions based on SoC ID. While >>>>> there's >>>>> certainly an argument for making the code support different SoCs at >>>>> run-time, the Tegra code is so far from that possible ideal that the >>>>> existing runtime code is an anomaly. If this changes in the future, all >>>>> runtime decisions should likely be based on DT anyway. >>>>> >>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>>> --- >>>>> arch/arm/mach-tegra/ap.c | 106 >>>>> ++++++++++----------------------- >>>>> arch/arm/mach-tegra/cache.c | 20 +++---- >>>>> arch/arm/mach-tegra/cpu.c | 16 ++--- >>>>> arch/arm/mach-tegra/cpu.h | 6 -- >>>>> arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- >>>>> 5 files changed, 51 insertions(+), 117 deletions(-) >>>> >>>> >>>> >>>> What exactly is missing to prevent multi-arch support? >>> >>> >>> In a word: everything:-) >>> >>> Pretty much all decisions in core architecture code, core Tegra code, >>> drivers, and even board files are currently made at compile time. For >>> example, consider drivers where the register layouts are different >>> between >>> different SoCs; not just new fields added, but existing fields moved to >>> different offsets. Right now, we handle this by changing the register >>> struct >>> definition at compile time. To support multiple chips, we'd have to >>> either >>> (a) link in n copies of the driver, one per register layout, or (b) >>> rework >>> the driver to use #defines and runtime calculations for register offsets, >>> like the Linux kernel drivers do. Tegra USB is one example. The pinmux >>> and >>> clock drivers have a significantly different sets of >>> pins/clocks/resets/... >>> per SoC, and enums/tables describing those sets are currently configured >>> at >>> compile time. Some PMIC constants (e.g. vdd_cpu voltage) are configured >>> at >>> compile-time, and even differ per board. >> >> >> I wonder how far we would get by converting clock, pinctrl, reset to >> driver model drivers? > > > Well, I expect we'll find out soon. The next SoC has radically different > clock/reset mechanisms, so we'll need to switch to standardized APIs for > clock/reset on Tegra to isolate drivers from those differences, and I > imagine that conversion would also involve conversion to DM since any > standard APIs probably assume use of DM. I haven't investigated this in > detail yet though. That sounds like a good move. > >>>> Shouldn't we head towards that rather than making it harder? >>> >>> >>> I don't see any need for that, no. >>> >>> U-Boot is built for a specific board (or in some cases a set of extremely >>> closely related set of boards, such as the RPI A/B/A+/B+). There's no >>> need >>> to determine almost anything at run-time since almost all information is >>> known at compile time, with exceptions such as standardized enumerable >>> buses >>> such as USB, PCIe. If we support multiple HW in a single binary, it gets >>> bloated with code that simply isn't going to be used, since all the extra >>> code is either for a platform that the build won't be installed on (e.g. >>> clock/pinmux tables), or is overhead to add runtime detection of which >>> block >>> of code to use, which simply isn't needed in the current model. >> >> >> It's not so much that. Presumably a build for a particular board would >> not include support for and SoC it doesn't have. But it is still >> useful to build the code. For example it would be nice to have an >> overall Tegra build that enables all SoCs to avoid building every >> board. >> >> So it is a serious question. I suspect the main impediment may be >> moving the clock and other core stuff to driver model. > > > Yes, everything is a bit too tightly coupled at the moment, and in many > cases each SoC-specific implementation exposes the same global symbols which > clients use. DM or similar conversions may well solve a lot of this. With a suitable API (even it if is pretty basic) the coupling can often go away. The current driver-model pinctrl/clock APIs are pretty simple, but they are effective in this regard for the SoCs I've tried. > >>> In my opinion, firmware/bootloaders run on a single specific board, >>> whereas >>> full-featured operating systems support multiple systems. >> >> >> Except when the boards are pretty similar. Also, doesn't barebox have >> only one build for Tegra? > > > I haven't looked at Barebox much. IIRC it only supports Tegra20 and not > later SoCs which could simplify things. Besides, I'm not arguing that it's > impossible to make a unified binary, simply that I don't see any need to do > so, except perhaps your compile-coverage suggestion. Sure. I think it might help push the APIs along though. I recall creating a clock API for tegra20 some years ago. I think if we go to the next level (clock drivers for each SoC) the SoC differences start to look a lot less. It's mostly this core code that is the problem and much of it should move to drivers IMO. > >>> As an aside, I've wondered whether U-Boot should be split into multiple >>> parts; one HW-specific binary providing various drivers (e.g. via >>> DM-related >>> APIs?) and the other containing just high-level user-interface code such >>> as >>> the shell, high-level USB/... protocols, which would only call into those >>> APIs. Still, I don't think we're anywhere close to that, and I'm not >>> aware >>> that it's a goal of the project at the moment. >> >> >> Well it gets built as one binary, but there's a pretty clear >> separation in the code, at least with driver model. What's the purpose >> of this? > > > It would allow the HW-agnostic portion to be compiled once (or once for a > CPU ISA) and re-used with any of the HW-/board-specific "driver" blobs. It'd > get use to "single binary" for the generic stuff, but without requiring the > for HW-specific code. Perhaps the generic portion could even run on top of > other driver stacks if they implemented the API it needed! However, this > does ignore potential feature differences in the common binary, e.g. someone > might want dfu/ums commands, but someone else might not need them and hence > consider them bloat. Still, those configurations would be differentiated by > feature more than HW, so it might still be useful. Sounds cool. It would certainly tidy up the U-Boot API a lot, I suppose. You could have a micro-module approach where every little feature is its own loadable module. But really, what is the benefit? Regards, Simon
On 04/29/2016 08:02 AM, Simon Glass wrote: > Hi Stephen, > > On 27 April 2016 at 10:13, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/27/2016 08:50 AM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 25 April 2016 at 13:25, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> On 04/23/2016 11:14 AM, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Stephen, >>>>> >>>>> On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>>> >>>>>> >>>>>> From: Stephen Warren <swarren@nvidia.com> >>>>>> >>>>>> U-Boot is compiled for a single board, which in turn uses a specific >>>>>> SoC. >>>>>> There's no need to make runtime decisions based on SoC ID. While >>>>>> there's >>>>>> certainly an argument for making the code support different SoCs at >>>>>> run-time, the Tegra code is so far from that possible ideal that the >>>>>> existing runtime code is an anomaly. If this changes in the future, all >>>>>> runtime decisions should likely be based on DT anyway. >>>>>> >>>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>>>> --- >>>>>> arch/arm/mach-tegra/ap.c | 106 ++++++++++----------------------- >>>>>> arch/arm/mach-tegra/cache.c | 20 +++---- >>>>>> arch/arm/mach-tegra/cpu.c | 16 ++--- >>>>>> arch/arm/mach-tegra/cpu.h | 6 -- >>>>>> arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- >>>>>> 5 files changed, 51 insertions(+), 117 deletions(-) >>>>> >>>>> >>>>> What exactly is missing to prevent multi-arch support? >>>> >>>> In a word: everything:-) >>>> >>>> Pretty much all decisions in core architecture code, core Tegra code, >>>> drivers, and even board files are currently made at compile time. For >>>> example, consider drivers where the register layouts are different between >>>> different SoCs; not just new fields added, but existing fields moved to >>>> different offsets. Right now, we handle this by changing the register struct >>>> definition at compile time. To support multiple chips, we'd have to either >>>> (a) link in n copies of the driver, one per register layout, or (b) rework >>>> the driver to use #defines and runtime calculations for register offsets, >>>> like the Linux kernel drivers do. Tegra USB is one example. The pinmux and >>>> clock drivers have a significantly different sets of pins/clocks/resets/... >>>> per SoC, and enums/tables describing those sets are currently configured at >>>> compile time. Some PMIC constants (e.g. vdd_cpu voltage) are configured at >>>> compile-time, and even differ per board. >>> >>> I wonder how far we would get by converting clock, pinctrl, reset to >>> driver model drivers? >> >> Well, I expect we'll find out soon. The next SoC has radically different >> clock/reset mechanisms, so we'll need to switch to standardized APIs for >> clock/reset on Tegra to isolate drivers from those differences, and I >> imagine that conversion would also involve conversion to DM since any >> standard APIs probably assume use of DM. I haven't investigated this in >> detail yet though. > > That sounds like a good move. I'm not sure if you still object to this patch for now, or would be happy giving it an ack?
Hi Stephen, On 29 April 2016 at 10:27, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/29/2016 08:02 AM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 27 April 2016 at 10:13, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>> On 04/27/2016 08:50 AM, Simon Glass wrote: >>>> >>>> >>>> Hi Stephen, >>>> >>>> On 25 April 2016 at 13:25, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> >>>>> >>>>> On 04/23/2016 11:14 AM, Simon Glass wrote: >>>>>> >>>>>> >>>>>> >>>>>> Hi Stephen, >>>>>> >>>>>> On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> >>>>>> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> From: Stephen Warren <swarren@nvidia.com> >>>>>>> >>>>>>> U-Boot is compiled for a single board, which in turn uses a specific >>>>>>> SoC. >>>>>>> There's no need to make runtime decisions based on SoC ID. While >>>>>>> there's >>>>>>> certainly an argument for making the code support different SoCs at >>>>>>> run-time, the Tegra code is so far from that possible ideal that the >>>>>>> existing runtime code is an anomaly. If this changes in the future, >>>>>>> all >>>>>>> runtime decisions should likely be based on DT anyway. >>>>>>> >>>>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>>>>> --- >>>>>>> arch/arm/mach-tegra/ap.c | 106 >>>>>>> ++++++++++----------------------- >>>>>>> arch/arm/mach-tegra/cache.c | 20 +++---- >>>>>>> arch/arm/mach-tegra/cpu.c | 16 ++--- >>>>>>> arch/arm/mach-tegra/cpu.h | 6 -- >>>>>>> arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- >>>>>>> 5 files changed, 51 insertions(+), 117 deletions(-) >>>>>> >>>>>> >>>>>> >>>>>> What exactly is missing to prevent multi-arch support? >>>>> >>>>> >>>>> In a word: everything:-) >>>>> >>>>> Pretty much all decisions in core architecture code, core Tegra code, >>>>> drivers, and even board files are currently made at compile time. For >>>>> example, consider drivers where the register layouts are different >>>>> between >>>>> different SoCs; not just new fields added, but existing fields moved to >>>>> different offsets. Right now, we handle this by changing the register >>>>> struct >>>>> definition at compile time. To support multiple chips, we'd have to >>>>> either >>>>> (a) link in n copies of the driver, one per register layout, or (b) >>>>> rework >>>>> the driver to use #defines and runtime calculations for register >>>>> offsets, >>>>> like the Linux kernel drivers do. Tegra USB is one example. The pinmux >>>>> and >>>>> clock drivers have a significantly different sets of >>>>> pins/clocks/resets/... >>>>> per SoC, and enums/tables describing those sets are currently >>>>> configured at >>>>> compile time. Some PMIC constants (e.g. vdd_cpu voltage) are configured >>>>> at >>>>> compile-time, and even differ per board. >>>> >>>> >>>> I wonder how far we would get by converting clock, pinctrl, reset to >>>> driver model drivers? >>> >>> >>> Well, I expect we'll find out soon. The next SoC has radically different >>> clock/reset mechanisms, so we'll need to switch to standardized APIs for >>> clock/reset on Tegra to isolate drivers from those differences, and I >>> imagine that conversion would also involve conversion to DM since any >>> standard APIs probably assume use of DM. I haven't investigated this in >>> detail yet though. >> >> >> That sounds like a good move. > > > I'm not sure if you still object to this patch for now, or would be happy > giving it an ack? Sorry, I still feel this is going in the wrong direction...CONFIG_TEGRA124 should mean that the code supports it, rather than the code exclusively supports it. In your commit message you say: "While there's certainly an argument for making the code support different SoCs at run-time, the Tegra code is so far from that possible ideal that the existing runtime code is an anomaly. If this changes in the future, all runtime decisions should likely be based on DT anyway." Or even build time.... That statement seems like saying that everything is so terrible that we might as well give up. What's your plan to move code into drivers? I am happy to help move things over and get towards the goal rather than further away. But really that should happen first. Also, the model that seems to be emerging is that SPL detects the platform and passes the correct DT to U-Boot proper. Specifically I think the end state should be: - Most SoC code is in drivers using driver model - It is possible to build most of the tegra code without building each SoC (just by having a 'tegra' board which enables all drivers) - If someone wanted to support multiple SoCs it would not be impossible to do so (even if mainline doesn't make it) - New SoCs are supported mostly by adding new drivers (although there is always of course some core/start-up code) Do you agree with that? Regards, Simon
Hi Stephen, On 29 April 2016 at 10:53, Simon Glass <sjg@chromium.org> wrote: > Hi Stephen, > > On 29 April 2016 at 10:27, Stephen Warren <swarren@wwwdotorg.org> wrote: >> On 04/29/2016 08:02 AM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 27 April 2016 at 10:13, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> On 04/27/2016 08:50 AM, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Stephen, >>>>> >>>>> On 25 April 2016 at 13:25, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>>> >>>>>> >>>>>> On 04/23/2016 11:14 AM, Simon Glass wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi Stephen, >>>>>>> >>>>>>> On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> From: Stephen Warren <swarren@nvidia.com> >>>>>>>> >>>>>>>> U-Boot is compiled for a single board, which in turn uses a specific >>>>>>>> SoC. >>>>>>>> There's no need to make runtime decisions based on SoC ID. While >>>>>>>> there's >>>>>>>> certainly an argument for making the code support different SoCs at >>>>>>>> run-time, the Tegra code is so far from that possible ideal that the >>>>>>>> existing runtime code is an anomaly. If this changes in the future, >>>>>>>> all >>>>>>>> runtime decisions should likely be based on DT anyway. >>>>>>>> >>>>>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>>>>>> --- >>>>>>>> arch/arm/mach-tegra/ap.c | 106 >>>>>>>> ++++++++++----------------------- >>>>>>>> arch/arm/mach-tegra/cache.c | 20 +++---- >>>>>>>> arch/arm/mach-tegra/cpu.c | 16 ++--- >>>>>>>> arch/arm/mach-tegra/cpu.h | 6 -- >>>>>>>> arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- >>>>>>>> 5 files changed, 51 insertions(+), 117 deletions(-) >>>>>>> >>>>>>> >>>>>>> >>>>>>> What exactly is missing to prevent multi-arch support? >>>>>> >>>>>> >>>>>> In a word: everything:-) >>>>>> >>>>>> Pretty much all decisions in core architecture code, core Tegra code, >>>>>> drivers, and even board files are currently made at compile time. For >>>>>> example, consider drivers where the register layouts are different >>>>>> between >>>>>> different SoCs; not just new fields added, but existing fields moved to >>>>>> different offsets. Right now, we handle this by changing the register >>>>>> struct >>>>>> definition at compile time. To support multiple chips, we'd have to >>>>>> either >>>>>> (a) link in n copies of the driver, one per register layout, or (b) >>>>>> rework >>>>>> the driver to use #defines and runtime calculations for register >>>>>> offsets, >>>>>> like the Linux kernel drivers do. Tegra USB is one example. The pinmux >>>>>> and >>>>>> clock drivers have a significantly different sets of >>>>>> pins/clocks/resets/... >>>>>> per SoC, and enums/tables describing those sets are currently >>>>>> configured at >>>>>> compile time. Some PMIC constants (e.g. vdd_cpu voltage) are configured >>>>>> at >>>>>> compile-time, and even differ per board. >>>>> >>>>> >>>>> I wonder how far we would get by converting clock, pinctrl, reset to >>>>> driver model drivers? >>>> >>>> >>>> Well, I expect we'll find out soon. The next SoC has radically different >>>> clock/reset mechanisms, so we'll need to switch to standardized APIs for >>>> clock/reset on Tegra to isolate drivers from those differences, and I >>>> imagine that conversion would also involve conversion to DM since any >>>> standard APIs probably assume use of DM. I haven't investigated this in >>>> detail yet though. >>> >>> >>> That sounds like a good move. >> >> >> I'm not sure if you still object to this patch for now, or would be happy >> giving it an ack? And just to be clear, I'm always keen to move things forward and I can see you have put a lot of work into this series. It would be easier to just apply it as is. What I am looking for is how we might get closer to the goal, perhaps after this series. This series has exposed this which is why I have brought it up. > > Sorry, I still feel this is going in the wrong > direction...CONFIG_TEGRA124 should mean that the code supports it, > rather than the code exclusively supports it. > > In your commit message you say: > > "While there's > certainly an argument for making the code support different SoCs at > run-time, the Tegra code is so far from that possible ideal that the > existing runtime code is an anomaly. If this changes in the future, all > runtime decisions should likely be based on DT anyway." > > Or even build time.... That statement seems like saying that > everything is so terrible that we might as well give up. > > What's your plan to move code into drivers? I am happy to help move > things over and get towards the goal rather than further away. But > really that should happen first. > > Also, the model that seems to be emerging is that SPL detects the > platform and passes the correct DT to U-Boot proper. > > Specifically I think the end state should be: > - Most SoC code is in drivers using driver model > - It is possible to build most of the tegra code without building each > SoC (just by having a 'tegra' board which enables all drivers) > - If someone wanted to support multiple SoCs it would not be > impossible to do so (even if mainline doesn't make it) > - New SoCs are supported mostly by adding new drivers (although there > is always of course some core/start-up code) > > Do you agree with that? > > Regards, > Simon Regards, Simon
On 04/29/2016 11:42 AM, Simon Glass wrote: > Hi Stephen, > > On 29 April 2016 at 10:53, Simon Glass <sjg@chromium.org> wrote: >> Hi Stephen, >> >> On 29 April 2016 at 10:27, Stephen Warren <swarren@wwwdotorg.org> wrote: >>> On 04/29/2016 08:02 AM, Simon Glass wrote: >>>> >>>> Hi Stephen, >>>> >>>> On 27 April 2016 at 10:13, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>> >>>>> On 04/27/2016 08:50 AM, Simon Glass wrote: >>>>>> >>>>>> >>>>>> Hi Stephen, >>>>>> >>>>>> On 25 April 2016 at 13:25, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>>>>> >>>>>>> >>>>>>> On 04/23/2016 11:14 AM, Simon Glass wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> Hi Stephen, >>>>>>>> >>>>>>>> On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> From: Stephen Warren <swarren@nvidia.com> >>>>>>>>> >>>>>>>>> U-Boot is compiled for a single board, which in turn uses a specific >>>>>>>>> SoC. >>>>>>>>> There's no need to make runtime decisions based on SoC ID. While >>>>>>>>> there's >>>>>>>>> certainly an argument for making the code support different SoCs at >>>>>>>>> run-time, the Tegra code is so far from that possible ideal that the >>>>>>>>> existing runtime code is an anomaly. If this changes in the future, >>>>>>>>> all >>>>>>>>> runtime decisions should likely be based on DT anyway. >>>>>>>>> >>>>>>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>>>>>>> --- >>>>>>>>> arch/arm/mach-tegra/ap.c | 106 >>>>>>>>> ++++++++++----------------------- >>>>>>>>> arch/arm/mach-tegra/cache.c | 20 +++---- >>>>>>>>> arch/arm/mach-tegra/cpu.c | 16 ++--- >>>>>>>>> arch/arm/mach-tegra/cpu.h | 6 -- >>>>>>>>> arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- >>>>>>>>> 5 files changed, 51 insertions(+), 117 deletions(-) >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> What exactly is missing to prevent multi-arch support? >>>>>>> >>>>>>> >>>>>>> In a word: everything:-) >>>>>>> >>>>>>> Pretty much all decisions in core architecture code, core Tegra code, >>>>>>> drivers, and even board files are currently made at compile time. For >>>>>>> example, consider drivers where the register layouts are different >>>>>>> between >>>>>>> different SoCs; not just new fields added, but existing fields moved to >>>>>>> different offsets. Right now, we handle this by changing the register >>>>>>> struct >>>>>>> definition at compile time. To support multiple chips, we'd have to >>>>>>> either >>>>>>> (a) link in n copies of the driver, one per register layout, or (b) >>>>>>> rework >>>>>>> the driver to use #defines and runtime calculations for register >>>>>>> offsets, >>>>>>> like the Linux kernel drivers do. Tegra USB is one example. The pinmux >>>>>>> and >>>>>>> clock drivers have a significantly different sets of >>>>>>> pins/clocks/resets/... >>>>>>> per SoC, and enums/tables describing those sets are currently >>>>>>> configured at >>>>>>> compile time. Some PMIC constants (e.g. vdd_cpu voltage) are configured >>>>>>> at >>>>>>> compile-time, and even differ per board. >>>>>> >>>>>> >>>>>> I wonder how far we would get by converting clock, pinctrl, reset to >>>>>> driver model drivers? >>>>> >>>>> >>>>> Well, I expect we'll find out soon. The next SoC has radically different >>>>> clock/reset mechanisms, so we'll need to switch to standardized APIs for >>>>> clock/reset on Tegra to isolate drivers from those differences, and I >>>>> imagine that conversion would also involve conversion to DM since any >>>>> standard APIs probably assume use of DM. I haven't investigated this in >>>>> detail yet though. >>>> >>>> >>>> That sounds like a good move. >>> >>> I'm not sure if you still object to this patch for now, or would be happy >>> giving it an ack? > > And just to be clear, I'm always keen to move things forward and I can > see you have put a lot of work into this series. It would be easier to > just apply it as is. What I am looking for is how we might get closer > to the goal, perhaps after this series. This series has exposed this > which is why I have brought it up. So, just to be clear: TomW can apply this (once I post V2) even if you're not giving an ack? >> Sorry, I still feel this is going in the wrong >> direction...CONFIG_TEGRA124 should mean that the code supports it, >> rather than the code exclusively supports it. That's certainly not what it means right now, and my intention in this series was not to address any short-comings in the current meanings of Kconfig. No matter what, if we do drive all this core SoC code from DT eventually, I don't imagine tegra_get_chip() will be the way code gets dispatched between multiple chips in the future, so removing it now shouldn't hurt any later conversion to runtime multi-chip support. In practice in a DT-driven world, I'd expect: - The code in enable_scu() to be part of some Tegra20-specific "SoC driver" or similar, and hence not need any runtime support restored. - The code in cache.c to be part of some specific cache drivers, which would only be instantiated on Tegra20, and hence not need any runtime support restored. - The code in cpu.c to be moved into a clock driver, and base on some per-SoC table in the clock driver, and hence not need any runtime special-casing. As such, I don't see reverting any of this patch being reverted later, and I don't believe leaving in these runtime calls would make it any easier to refactor the code into drivers later. >> In your commit message you say: >> >> "While there's >> certainly an argument for making the code support different SoCs at >> run-time, the Tegra code is so far from that possible ideal that the >> existing runtime code is an anomaly. If this changes in the future, all >> runtime decisions should likely be based on DT anyway." >> >> Or even build time.... That statement seems like saying that >> everything is so terrible that we might as well give up. I'm just being practical. Please note in particular the comment about basing such decisions on DT; that and refactoring the code into DT-instantiated drivers would leave it so different that the tiny number of current runtime switches doesn't help anything. >> What's your plan to move code into drivers? I am happy to help move >> things over and get towards the goal rather than further away. But >> really that should happen first. I don't have a plan for anything besides the clock and reset drivers at the moment. I /think/ that most of the rest of the code in arch/arm/mach-tegra simply won't be needed on Tegra186. We'll see; I only have UART and GPIO working right now, well and a very hacked-up MMC to avoid clock/reset access. There is a large amount of work left that I haven't started looking at yet. >> Also, the model that seems to be emerging is that SPL detects the >> platform and passes the correct DT to U-Boot proper. There is no SPL on Tegra210 or later. It would be a pity to have to introduce one just to do runtime DT selection when there's no need to have runtime DT selection. >> Specifically I think the end state should be: >> - Most SoC code is in drivers using driver model That is fine in many cases. There's plenty of low-level bootstrap code where I'm not sure that it's worth it. Equally, given that you have stated DM==DT, I worry about the implications of that statement if universally applied, rather than selectively/pragmatically applied. >> - It is possible to build most of the tegra code without building each >> SoC (just by having a 'tegra' board which enables all drivers) I'm not convinced this is worth it universally. For the purposes of actually running a binary on a device, I believe there is zero need for this. For compile coverage it might be nice, especially for driver code, although running buildman on one Tegra board for each SoC generation doesn't take much time. Even for drivers that solely use generic (non-SoC-version-specific) APIs, this would be difficult in some cases. This plays back into my earlier comments elsewhere about structs-vs-defines for register address calculations. Register layouts of some devices vary incompatibly between Tegra SoC generations. With a define-based driver, this can all be handled at run-time. With a struct-based driver, one must compile the code once per register layout. Rather than invest in making a "compile test" build that does this, which would require various build system work, it's much simpler to just compile the driver multiple times as part of a set of board builds, which already works today. The result is still compiling the code n times, so there wouldn't be much time saving wrapping everything into one build. >> - If someone wanted to support multiple SoCs it would not be >> impossible to do so (even if mainline doesn't make it) I don't think it's a good idea to support features that mainline doesn't use. They'll bit-rot and not work anyway. I also can't see any use-case for such a build. >> - New SoCs are supported mostly by adding new drivers (although there >> is always of course some core/start-up code) Yes, it certainly makes sense to use drivers for functionality which other pieces of the code must interact with in a generic way. For example, calls from I2C/MMC drivers to clock/reset APIs. I expect some debate over what code is core/start-up code though. For example, static pinmux setup fits that category for me, since there's no generic code that needs to interface with it.
Hi Stephen, On 29 April 2016 at 13:21, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 04/29/2016 11:42 AM, Simon Glass wrote: >> >> Hi Stephen, >> >> On 29 April 2016 at 10:53, Simon Glass <sjg@chromium.org> wrote: >>> >>> Hi Stephen, >>> >>> On 29 April 2016 at 10:27, Stephen Warren <swarren@wwwdotorg.org> wrote: >>>> >>>> On 04/29/2016 08:02 AM, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Stephen, >>>>> >>>>> On 27 April 2016 at 10:13, Stephen Warren <swarren@wwwdotorg.org> >>>>> wrote: >>>>>> >>>>>> >>>>>> On 04/27/2016 08:50 AM, Simon Glass wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> Hi Stephen, >>>>>>> >>>>>>> On 25 April 2016 at 13:25, Stephen Warren <swarren@wwwdotorg.org> >>>>>>> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 04/23/2016 11:14 AM, Simon Glass wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> Hi Stephen, >>>>>>>>> >>>>>>>>> On 19 April 2016 at 14:59, Stephen Warren <swarren@wwwdotorg.org> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> From: Stephen Warren <swarren@nvidia.com> >>>>>>>>>> >>>>>>>>>> U-Boot is compiled for a single board, which in turn uses a >>>>>>>>>> specific >>>>>>>>>> SoC. >>>>>>>>>> There's no need to make runtime decisions based on SoC ID. While >>>>>>>>>> there's >>>>>>>>>> certainly an argument for making the code support different SoCs >>>>>>>>>> at >>>>>>>>>> run-time, the Tegra code is so far from that possible ideal that >>>>>>>>>> the >>>>>>>>>> existing runtime code is an anomaly. If this changes in the >>>>>>>>>> future, >>>>>>>>>> all >>>>>>>>>> runtime decisions should likely be based on DT anyway. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Stephen Warren <swarren@nvidia.com> >>>>>>>>>> --- >>>>>>>>>> arch/arm/mach-tegra/ap.c | 106 >>>>>>>>>> ++++++++++----------------------- >>>>>>>>>> arch/arm/mach-tegra/cache.c | 20 +++---- >>>>>>>>>> arch/arm/mach-tegra/cpu.c | 16 ++--- >>>>>>>>>> arch/arm/mach-tegra/cpu.h | 6 -- >>>>>>>>>> arch/arm/mach-tegra/tegra20/warmboot.c | 20 ++----- >>>>>>>>>> 5 files changed, 51 insertions(+), 117 deletions(-) >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> What exactly is missing to prevent multi-arch support? >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> In a word: everything:-) >>>>>>>> >>>>>>>> Pretty much all decisions in core architecture code, core Tegra >>>>>>>> code, >>>>>>>> drivers, and even board files are currently made at compile time. >>>>>>>> For >>>>>>>> example, consider drivers where the register layouts are different >>>>>>>> between >>>>>>>> different SoCs; not just new fields added, but existing fields moved >>>>>>>> to >>>>>>>> different offsets. Right now, we handle this by changing the >>>>>>>> register >>>>>>>> struct >>>>>>>> definition at compile time. To support multiple chips, we'd have to >>>>>>>> either >>>>>>>> (a) link in n copies of the driver, one per register layout, or (b) >>>>>>>> rework >>>>>>>> the driver to use #defines and runtime calculations for register >>>>>>>> offsets, >>>>>>>> like the Linux kernel drivers do. Tegra USB is one example. The >>>>>>>> pinmux >>>>>>>> and >>>>>>>> clock drivers have a significantly different sets of >>>>>>>> pins/clocks/resets/... >>>>>>>> per SoC, and enums/tables describing those sets are currently >>>>>>>> configured at >>>>>>>> compile time. Some PMIC constants (e.g. vdd_cpu voltage) are >>>>>>>> configured >>>>>>>> at >>>>>>>> compile-time, and even differ per board. >>>>>>> >>>>>>> >>>>>>> >>>>>>> I wonder how far we would get by converting clock, pinctrl, reset to >>>>>>> driver model drivers? >>>>>> >>>>>> >>>>>> >>>>>> Well, I expect we'll find out soon. The next SoC has radically >>>>>> different >>>>>> clock/reset mechanisms, so we'll need to switch to standardized APIs >>>>>> for >>>>>> clock/reset on Tegra to isolate drivers from those differences, and I >>>>>> imagine that conversion would also involve conversion to DM since any >>>>>> standard APIs probably assume use of DM. I haven't investigated this >>>>>> in >>>>>> detail yet though. >>>>> >>>>> >>>>> >>>>> That sounds like a good move. >>>> >>>> >>>> I'm not sure if you still object to this patch for now, or would be >>>> happy >>>> giving it an ack? >> >> >> And just to be clear, I'm always keen to move things forward and I can >> see you have put a lot of work into this series. It would be easier to >> just apply it as is. What I am looking for is how we might get closer >> to the goal, perhaps after this series. This series has exposed this >> which is why I have brought it up. > > > So, just to be clear: TomW can apply this (once I post V2) even if you're > not giving an ack? Provided you are OK with using drivers for clock, pinctrl etc. with your new Tegra work. I'd really like to move away from all this code in arch/arm, private APIs, etc.. > >>> Sorry, I still feel this is going in the wrong >>> direction...CONFIG_TEGRA124 should mean that the code supports it, >>> rather than the code exclusively supports it. > > > That's certainly not what it means right now, and my intention in this > series was not to address any short-comings in the current meanings of > Kconfig. Well it means that in quite a few places - e.g. this one that you are removing. > > No matter what, if we do drive all this core SoC code from DT eventually, I > don't imagine tegra_get_chip() will be the way code gets dispatched between > multiple chips in the future, so removing it now shouldn't hurt any later > conversion to runtime multi-chip support. > > In practice in a DT-driven world, I'd expect: > > - The code in enable_scu() to be part of some Tegra20-specific "SoC driver" > or similar, and hence not need any runtime support restored. > > - The code in cache.c to be part of some specific cache drivers, which would > only be instantiated on Tegra20, and hence not need any runtime support > restored. > > - The code in cpu.c to be moved into a clock driver, and base on some > per-SoC table in the clock driver, and hence not need any runtime > special-casing. > > As such, I don't see reverting any of this patch being reverted later, and I > don't believe leaving in these runtime calls would make it any easier to > refactor the code into drivers later. Sounds reasonable. So long as we are agreed on using drivers then I am OK with it as a means to an end. > >>> In your commit message you say: >>> >>> "While there's >>> certainly an argument for making the code support different SoCs at >>> run-time, the Tegra code is so far from that possible ideal that the >>> existing runtime code is an anomaly. If this changes in the future, all >>> runtime decisions should likely be based on DT anyway." >>> >>> Or even build time.... That statement seems like saying that >>> everything is so terrible that we might as well give up. > > > I'm just being practical. Please note in particular the comment about basing > such decisions on DT; that and refactoring the code into DT-instantiated > drivers would leave it so different that the tiny number of current runtime > switches doesn't help anything. Right, but you are actually removing functions that detect the SoC. That is surely heading the wrong way. > >>> What's your plan to move code into drivers? I am happy to help move >>> things over and get towards the goal rather than further away. But >>> really that should happen first. > > > I don't have a plan for anything besides the clock and reset drivers at the > moment. I /think/ that most of the rest of the code in arch/arm/mach-tegra > simply won't be needed on Tegra186. We'll see; I only have UART and GPIO > working right now, well and a very hacked-up MMC to avoid clock/reset > access. There is a large amount of work left that I haven't started looking > at yet. OK good. > >>> Also, the model that seems to be emerging is that SPL detects the >>> platform and passes the correct DT to U-Boot proper. > > > There is no SPL on Tegra210 or later. It would be a pity to have to > introduce one just to do runtime DT selection when there's no need to have > runtime DT selection. How does the platform start up? Does it use BCT and then jump straight into U-Boot? > >>> Specifically I think the end state should be: >>> - Most SoC code is in drivers using driver model > > > That is fine in many cases. There's plenty of low-level bootstrap code where > I'm not sure that it's worth it. Equally, given that you have stated DM==DT, > I worry about the implications of that statement if universally applied, > rather than selectively/pragmatically applied. I have not stated that DM==DT. Where there are reasons to use platform data we can do that. See this comment in the driver-model README: *** Note: platform data is the old way of doing things. It is *** basically a C structure which is passed to drivers to tell them about *** platform-specific settings like the address of its registers, bus *** speed, etc. Device tree is now the preferred way of handling this. *** Unless you have a good reason not to use device tree (the main one *** being you need serial support in SPL and don't have enough SRAM for *** the cut-down device tree and libfdt libraries) you should stay away *** from platform data. This is a boot loader. We need to be pragmatic. This is why I push back on core driver-model code bloat. > >>> - It is possible to build most of the tegra code without building each >>> SoC (just by having a 'tegra' board which enables all drivers) > > > I'm not convinced this is worth it universally. > > For the purposes of actually running a binary on a device, I believe there > is zero need for this. > > For compile coverage it might be nice, especially for driver code, although > running buildman on one Tegra board for each SoC generation doesn't take > much time. > > Even for drivers that solely use generic (non-SoC-version-specific) APIs, > this would be difficult in some cases. This plays back into my earlier > comments elsewhere about structs-vs-defines for register address > calculations. Register layouts of some devices vary incompatibly between > Tegra SoC generations. With a define-based driver, this can all be handled > at run-time. With a struct-based driver, one must compile the code once per > register layout. Rather than invest in making a "compile test" build that > does this, which would require various build system work, it's much simpler > to just compile the driver multiple times as part of a set of board builds, > which already works today. The result is still compiling the code n times, > so there wouldn't be much time saving wrapping everything into one build. Well your hardware guys really should make up their mind. Is there any code review? And s/w people should have a system (with serial driver) running before tape-out and catch this stuff. But in any case, struct-based stuff is not a requirement now. I believe a lot of people prefer it (including me) but in the case you raise it isn't practical. See ns16550 for how this can be done badly. > >>> - If someone wanted to support multiple SoCs it would not be >>> impossible to do so (even if mainline doesn't make it) > > > I don't think it's a good idea to support features that mainline doesn't > use. They'll bit-rot and not work anyway. I also can't see any use-case for > such a build. While SoC vendors think of the SoC as a the most important factor in the platform, we are starting to see similar devices using different SoCs, or at least different variants. From a user point of view I'd rather not worry about which Raspberry Pi I have, for example, so long as I can build something that boots. > >>> - New SoCs are supported mostly by adding new drivers (although there >>> is always of course some core/start-up code) > > > Yes, it certainly makes sense to use drivers for functionality which other > pieces of the code must interact with in a generic way. For example, calls > from I2C/MMC drivers to clock/reset APIs. > > I expect some debate over what code is core/start-up code though. For > example, static pinmux setup fits that category for me, since there's no > generic code that needs to interface with it. So please put your pinmux code in a pinctrl driver, even it is just in the probe() method. Also I have a Tegra124 platform now. Once the smoke has cleared with your work I'd like to take a look at how to improve things. Regards, Simon
diff --git a/arch/arm/mach-tegra/ap.c b/arch/arm/mach-tegra/ap.c index a200010e2937..a8e3e8b7dfcb 100644 --- a/arch/arm/mach-tegra/ap.c +++ b/arch/arm/mach-tegra/ap.c @@ -10,7 +10,6 @@ #include <common.h> #include <linux/bug.h> #include <asm/io.h> -#include <asm/arch/gp_padctrl.h> #include <asm/arch-tegra/clock.h> #include <asm/arch-tegra/tegra.h> #include <soc/mc.h> @@ -19,23 +18,6 @@ #include "pmc.h" #include "scu.h" -int tegra_get_chip(void) -{ - int rev; - struct apb_misc_gp_ctlr *gp = - (struct apb_misc_gp_ctlr *)NV_PA_APB_MISC_GP_BASE; - - /* - * This is undocumented, Chip ID is bits 15:8 of the register - * APB_MISC + 0x804, and has value 0x20 for Tegra20, 0x30 for - * Tegra30, 0x35 for T114, and 0x40 for Tegra124. - */ - rev = (readl(&gp->hidrev) & HIDREV_CHIPID_MASK) >> HIDREV_CHIPID_SHIFT; - debug("%s: CHIPID is 0x%02X\n", __func__, rev); - - return rev; -} - int tegra_get_sku_info(void) { int sku_id; @@ -49,74 +31,47 @@ int tegra_get_sku_info(void) int tegra_get_chip_sku(void) { - uint sku_id, chip_id; +#ifdef CONFIG_TEGRA20 + uint sku_id; - chip_id = tegra_get_chip(); sku_id = tegra_get_sku_info(); - - switch (chip_id) { - case CHIPID_TEGRA20: - switch (sku_id) { - case SKU_ID_T20_7: - case SKU_ID_T20: - return TEGRA_SOC_T20; - case SKU_ID_T25SE: - case SKU_ID_AP25: - case SKU_ID_T25: - case SKU_ID_AP25E: - case SKU_ID_T25E: - return TEGRA_SOC_T25; - } - break; - case CHIPID_TEGRA30: - switch (sku_id) { - case SKU_ID_T33: - case SKU_ID_T30: - case SKU_ID_TM30MQS_P_A3: - default: - return TEGRA_SOC_T30; - } - break; - case CHIPID_TEGRA114: - switch (sku_id) { - case SKU_ID_T114_ENG: - case SKU_ID_T114_1: - default: - return TEGRA_SOC_T114; - } - break; - case CHIPID_TEGRA124: - switch (sku_id) { - case SKU_ID_T124_ENG: - default: - return TEGRA_SOC_T124; - } - break; - case CHIPID_TEGRA210: - switch (sku_id) { - case SKU_ID_T210_ENG: - default: - return TEGRA_SOC_T210; - } - break; + switch (sku_id) { + case SKU_ID_T20_7: + case SKU_ID_T20: + return TEGRA_SOC_T20; + case SKU_ID_T25SE: + case SKU_ID_AP25: + case SKU_ID_T25: + case SKU_ID_AP25E: + case SKU_ID_T25E: + return TEGRA_SOC_T25; + default: + /* unknown chip/sku id */ + printf("ERROR: UNKNOWN SKU ID 0x%02X\n", sku_id); + return TEGRA_SOC_UNKNOWN; } - - /* unknown chip/sku id */ - printf("%s: ERROR: UNKNOWN CHIP/SKU ID COMBO (0x%02X/0x%02X)\n", - __func__, chip_id, sku_id); - return TEGRA_SOC_UNKNOWN; +#endif +#ifdef CONFIG_TEGRA30 + return TEGRA_SOC_T30; +#endif +#ifdef CONFIG_TEGRA114 + return TEGRA_SOC_T114; +#endif +#ifdef CONFIG_TEGRA124 + return TEGRA_SOC_T124; +#endif +#ifdef CONFIG_TEGRA210 + return TEGRA_SOC_T210; +#endif } #ifndef CONFIG_ARM64 static void enable_scu(void) { +#ifdef CONFIG_TEGRA20 struct scu_ctlr *scu = (struct scu_ctlr *)NV_PA_ARM_PERIPHBASE; u32 reg; - /* Only enable the SCU on T20/T25 */ - if (tegra_get_chip() != CHIPID_TEGRA20) - return; - /* If SCU already setup/enabled, return */ if (readl(&scu->scu_ctrl) & SCU_CTRL_ENABLE) return; @@ -128,6 +83,7 @@ static void enable_scu(void) reg = readl(&scu->scu_ctrl); reg |= SCU_CTRL_ENABLE; writel(reg, &scu->scu_ctrl); +#endif } static u32 get_odmdata(void) diff --git a/arch/arm/mach-tegra/cache.c b/arch/arm/mach-tegra/cache.c index b93814fcb96b..a41a7b9e2868 100644 --- a/arch/arm/mach-tegra/cache.c +++ b/arch/arm/mach-tegra/cache.c @@ -8,31 +8,29 @@ #include <common.h> #include <asm/io.h> -#include <asm/arch/gp_padctrl.h> #include "cpu.h" #ifndef CONFIG_ARM64 void config_cache(void) { - u32 reg = 0; - /* enable SMP mode and FW for CPU0, by writing to Auxiliary Ctl reg */ asm volatile( "mrc p15, 0, r0, c1, c0, 1\n" "orr r0, r0, #0x41\n" "mcr p15, 0, r0, c1, c0, 1\n"); - /* Currently, only Tegra114+ needs this L2 cache change to boot Linux */ - if (tegra_get_chip() < CHIPID_TEGRA114) - return; - +#if defined(CONFIG_TEGRA114) || defined(CONFIG_TEGRA124) /* * Systems with an architectural L2 cache must not use the PL310. * Config L2CTLR here for a data RAM latency of 3 cycles. */ - asm("mrc p15, 1, %0, c9, c0, 2" : : "r" (reg)); - reg &= ~7; - reg |= 2; - asm("mcr p15, 1, %0, c9, c0, 2" : : "r" (reg)); + { + u32 reg; + asm("mrc p15, 1, %0, c9, c0, 2" : "=r" (reg)); + reg &= ~7; + reg |= 2; + asm("mcr p15, 1, %0, c9, c0, 2" : : "r" (reg)); + } +#endif } #endif diff --git a/arch/arm/mach-tegra/cpu.c b/arch/arm/mach-tegra/cpu.c index 3562a2d2188a..cc60908677a1 100644 --- a/arch/arm/mach-tegra/cpu.c +++ b/arch/arm/mach-tegra/cpu.c @@ -172,7 +172,6 @@ int pllx_set_rate(struct clk_pll_simple *pll , u32 divn, u32 divm, u32 divp, u32 cpcon) { struct clk_pll_info *pllinfo = &tegra_pll_info_table[CLOCK_ID_XCPU]; - int chip = tegra_get_chip(); u32 reg; debug("%s entry\n", __func__); @@ -189,11 +188,12 @@ int pllx_set_rate(struct clk_pll_simple *pll , u32 divn, u32 divm, reg |= (divn << pllinfo->n_shift) | (divp << pllinfo->p_shift); writel(reg, &pll->pll_base); + reg = 0; + +#if defined(CONFIG_TEGRA20) || defined(CONFIG_TEGRA30) /* Set cpcon to PLLX_MISC */ - if (chip == CHIPID_TEGRA20 || chip == CHIPID_TEGRA30) - reg = (cpcon << pllinfo->kcp_shift); - else - reg = 0; + reg |= (cpcon << pllinfo->kcp_shift); +#endif /* * TODO(twarren@nvidia.com) Check which SoCs use DCCON @@ -230,15 +230,11 @@ void init_pllx(void) { struct clk_rst_ctlr *clkrst = (struct clk_rst_ctlr *)NV_PA_CLK_RST_BASE; struct clk_pll_simple *pll = &clkrst->crc_pll_simple[SIMPLE_PLLX]; - int soc_type, sku_info, chip_sku; + int sku_info, chip_sku; enum clock_osc_freq osc; struct clk_pll_table *sel; debug("%s entry\n", __func__); - /* get SOC (chip) type */ - soc_type = tegra_get_chip(); - debug("%s: SoC = 0x%02X\n", __func__, soc_type); - /* get SKU info */ sku_info = tegra_get_sku_info(); debug("%s: SKU info byte = 0x%02X\n", __func__, sku_info); diff --git a/arch/arm/mach-tegra/cpu.h b/arch/arm/mach-tegra/cpu.h index f1f5b179c198..ee4f7868baf7 100644 --- a/arch/arm/mach-tegra/cpu.h +++ b/arch/arm/mach-tegra/cpu.h @@ -64,12 +64,6 @@ void powerup_cpu(void); void reset_A9_cpu(int reset); void start_cpu(u32 reset_vector); /** - * Returns the pure SOC (chip ID) from the HIDREV register - * - * @return SOC ID - see CHIPID_TEGRAxx... - */ -int tegra_get_chip(void); -/** * Returns the SKU ID from the sku_info register * * @return SKU ID - see SKU_ID_Txx... diff --git a/arch/arm/mach-tegra/tegra20/warmboot.c b/arch/arm/mach-tegra/tegra20/warmboot.c index 9d67bdef494a..1159f05a5dc3 100644 --- a/arch/arm/mach-tegra/tegra20/warmboot.c +++ b/arch/arm/mach-tegra/tegra20/warmboot.c @@ -242,22 +242,12 @@ static int ap20_is_production_mode(void) static enum fuse_operating_mode fuse_get_operation_mode(void) { - u32 chip_id; - struct apb_misc_gp_ctlr *gp = - (struct apb_misc_gp_ctlr *)NV_PA_APB_MISC_GP_BASE; - - chip_id = (readl(&gp->hidrev) & HIDREV_CHIPID_MASK) >> - HIDREV_CHIPID_SHIFT; - if (chip_id == CHIPID_TEGRA20) { - if (ap20_is_odm_production_mode()) { - printf("!! odm_production_mode is not supported !!\n"); - return MODE_UNDEFINED; - } else - if (ap20_is_production_mode()) - return MODE_PRODUCTION; - else - return MODE_UNDEFINED; + if (ap20_is_odm_production_mode()) { + printf("!! odm_production_mode is not supported !!\n"); + return MODE_UNDEFINED; } + if (ap20_is_production_mode()) + return MODE_PRODUCTION; return MODE_UNDEFINED; }