On Sat, May 24, 2025 at 04:29:05PM +0100, Simon Glass wrote: > Hi Tom, > > On Sat, 24 May 2025 at 15:53, Tom Rini <tr...@konsulko.com> wrote: > > > > On Sat, May 24, 2025 at 03:31:08PM +0100, Simon Glass wrote: > > > Hi Tom, > > > > > > On Thu, 15 May 2025 at 21:54, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > On Thu, May 15, 2025 at 09:26:42PM +0200, Simon Glass wrote: > > > > > Hi Tom, > > > > > > > > > > On Wed, 14 May 2025 at 23:58, Tom Rini <tr...@konsulko.com> wrote: > > > > > > > > > > > > On Sat, May 10, 2025 at 03:42:07PM +0200, Simon Glass wrote: > > > > > > > > > > > > > Accept a bloblist and control devicetree from a previous phase in > > > > > > > registers 0 to 3, as documented in the Firmware Handoff > > > > > > > specification[1]. > > > > > > > > > > > > > > Note that there is currently a weak function called > > > > > > > save_boot_params() > > > > > > > which happens to save the same registers as are needed for this > > > > > > > protocol. But it seems better to implement this feature as a > > > > > > > first-class > > > > > > > citizen, if for no other reason than that many boards have their > > > > > > > own > > > > > > > version of save_boot_params() which may not happen to save the > > > > > > > required > > > > > > > registers. A weak function is not a good basis for an API. > > > > > > > > > > > > > > [1] > > > > > > > https://firmwarehandoff.github.io/firmware_handoff/main/index.html > > > > > > > > > > > > > > Signed-off-by: Simon Glass <s...@chromium.org> > > > > > > > --- > > > > > > > While some efforts were made to encourage Intel to adopt this > > > > > > > specification, this has not yet been successful. However I do > > > > > > > intend to > > > > > > > use my proposed x86 handoff in U-Boot, once this series is in. > > > > > > > > > > > > > > Changes in v4: > > > > > > > - Update commit message to indicate this can only be for ARM at > > > > > > > present > > > > > > > - Update commit message to mention why save_boot_params() is not > > > > > > > used > > > > > > > > > > > > > > Changes in v3: > > > > > > > - Add support for aarch64 also > > > > > > > - Update registers to match the Firmware Handoff protocol > > > > > > > > > > > > > > Changes in v2: > > > > > > > - Use three registers instead of two for the entry > > > > > > > > > > > > > > arch/arm/cpu/armv7/start.S | 10 +++++++++- > > > > > > > arch/arm/cpu/armv8/start.S | 12 ++++++++++++ > > > > > > > arch/arm/lib/crt0.S | 6 ++++++ > > > > > > > arch/arm/lib/crt0_64.S | 6 ++++++ > > > > > > > 4 files changed, 33 insertions(+), 1 deletion(-) > > > > > > > > > > > > This is what makes me think you aren't actually working on my > > > > > > feedback > > > > > > about taking what we have in-tree, now, rather than just forward > > > > > > porting > > > > > > your old series and deleting the current code. > > > > > > > > > > Well, yes, I sent this v3 series because I had thought you were > > > > > willing to allow me to maintain bloblist. I certainly appreciate the > > > > > work in this area, though. > > > > > > > > > > > > > > > > > We have save_boot_params doing exactly what it should today, > > > > > > including > > > > > > saving off a standard passage and making what was saved available > > > > > > later, > > > > > > in an intentional fashion. If, hypothetically, Pi needs to override > > > > > > save_boot_param because a future new-firmware-blob does standard > > > > > > passage > > > > > > but at run time we want to support both, that's a good thing. > > > > > > > > > > > > With commit ea3348ebc215 ('Merge patch series "Handoff bloblist from > > > > > > previous boot stage"') we already implement the specification in > > > > > > [1]. It > > > > > > is not a "which happens to". It is intentional. > > > > > > > > > > > > I guess the overall challenge here is that you want to do things > > > > > > which > > > > > > are a superset of the firmware handoff spec itself? > > > > > > > > > > Yes,, much of this series relates to putting some structure into > > > > > things which are passed via standard passage, including validation. > > > > > > > > > > I actually don't understand why you would be keen on using > > > > > save_boot_params(), a weak function, used for board-specific features, > > > > > to implement standard passage. We should build standard passage in as > > > > > a first-class citizen. I certainly don't want to try to implement > > > > > things that way on x86. > > > > > > > > You seem to have misunderstood a lot of things and perhaps need to go > > > > and re-read the feedback. Nothing about a weak function implies less > > > > than "first class" and "maintain the code" does not equate "delete and > > > > replace functional code with the same thing, but I wrote it". And if you > > > > aren't going to do that, please don't bother posting a v4 either later > > > > this week. I feel like I'm having zero progress with you taking my > > > > feedback on anything at this point. > > > > > > Are you completely set on the idea of a weak function here? > > > > Am I completely set on following the general architecture and design we > > have and not making needless changes? Again, you seem to have either > > misread not not understood feedback and are intent on doing what you > > want anyhow. > > The save_boot_params() mechanism is ARM-only and is not suitable for > standard passage, IMO... > > So can you just tell me whether you are willing to have the registers > saved in global_data early in startup, in assembly code? I want that > before progressing with standard passage on x86. I don't need to say > yes. I just need a clear answer, so I can decide which path to take.
I already replied in another thread that fine, we can try your idea. But I'm going to note here that trying to write to global data before it exists seems like a bad idea. Something is only "ARM-only" by virtue of being in assembly because it's too early to be able to use a C function. -- Tom
signature.asc
Description: PGP signature
