[U-Boot] [PATCH 38/60] ARM: tegra: remove tegra_get_chip()

[Stephen Warren <swarren@wwwdotorg.org>]

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.