Martin Roth has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
soc/amd/picasso: Add dummy spinlock for psp_verstage
If CONFIG_CMOS_POST is enabled, psp_verstage breaks because the spinlock code is missing. Add dummy spinlock code as the spinlocks aren't needed in the PSP.
TEST=Build with CONFIG_CMOS_POST enabled. BUG=None
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: Iea6f31e500e1b26f0b974c6eaa486209b9c81459 --- A src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h 1 file changed, 17 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/10/43310/1
diff --git a/src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h b/src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h new file mode 100644 index 0000000..5245bd1 --- /dev/null +++ b/src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _ARCH_SMP_SPINLOCK_H +#define _ARCH_SMP_SPINLOCK_H + +#define DECLARE_SPIN_LOCK(x) +#define barrier() do {} while (0) +#define spin_is_locked(lock) 0 +#define spin_unlock_wait(lock) do {} while (0) +#define spin_lock(lock) do {} while (0) +#define spin_unlock(lock) do {} while (0) +#define cpu_relax() do {} while (0) + +#include <smp/node.h> +#define boot_cpu() 1 + +#endif
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 1: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 1: Code-Review+2
Marshall Dawson has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 1: Code-Review+2
Martin Roth has submitted this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
soc/amd/picasso: Add dummy spinlock for psp_verstage
If CONFIG_CMOS_POST is enabled, psp_verstage breaks because the spinlock code is missing. Add dummy spinlock code as the spinlocks aren't needed in the PSP.
TEST=Build with CONFIG_CMOS_POST enabled. BUG=None
Signed-off-by: Martin Roth martin@coreboot.org Change-Id: Iea6f31e500e1b26f0b974c6eaa486209b9c81459 Reviewed-on: https://review.coreboot.org/c/coreboot/+/43310 Reviewed-by: Raul Rangel rrangel@chromium.org Reviewed-by: Karthik Ramasubramanian kramasub@google.com Reviewed-by: Marshall Dawson marshalldawson3rd@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- A src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h 1 file changed, 17 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Marshall Dawson: Looks good to me, approved Raul Rangel: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h b/src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h new file mode 100644 index 0000000..5245bd1 --- /dev/null +++ b/src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h @@ -0,0 +1,17 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef _ARCH_SMP_SPINLOCK_H +#define _ARCH_SMP_SPINLOCK_H + +#define DECLARE_SPIN_LOCK(x) +#define barrier() do {} while (0) +#define spin_is_locked(lock) 0 +#define spin_unlock_wait(lock) do {} while (0) +#define spin_lock(lock) do {} while (0) +#define spin_unlock(lock) do {} while (0) +#define cpu_relax() do {} while (0) + +#include <smp/node.h> +#define boot_cpu() 1 + +#endif
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
So.. two hours and five minutes from publish to submit?
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1 This is confusing since <smp/node.h> declares
int boot_cpu(void);
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
Patch Set 2:
(1 comment)
So.. two hours and five minutes from publish to submit?
Yes, this was to fix a breakage that would have hit in chromeos, and we followed the procedure by getting 3 +2s.
https://doc.coreboot.org/getting_started/gerrit_guidelines.html#more-detail
"Let non-trivial patches sit in a review state for at least 24 hours before submission. ... Patches can be ‘Fast-tracked’ and submitted in under 24 hours with the agreement of at least 3 +2 votes."
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
This is confusing since <smp/node.h> declares […]
Can we fix these up by having a notion of STAGE_SUPPORTS_SMP ? That we can provide default implementations for all these in the include/smp/spinlock.h. We nearly have that aside from CONFIG(SMP) is global. I think we could probably just add a Kconfig per stage that indicates if STAGE_SUPPORTS_SMP or STAGE_SINGLE_CPU or some such. What do you think?
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
Patch Set 2:
Patch Set 2:
(1 comment)
So.. two hours and five minutes from publish to submit?
Yes, this was to fix a breakage that would have hit in chromeos, and we followed the procedure by getting 3 +2s.
Typically this 3x +2 was only pulled for breakage that already had happened in master.
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
Can we fix these up by having a notion of STAGE_SUPPORTS_SMP ? That we can provide default implement […]
Well that would be preferred. My questiona are really:
1) Why do whe have the <smp/node.h> include here? 2) We have symmetry expectations for headers and <smp/spinlock.h> is not the one to provide boot_cpu() implementation. 3) Commit message completely ignores adding boot_cpu(). The connection with spinlocks is too remote to omit that. 4) Do we really want to allow preprocess alias proper function prototypes?
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
Well that would be preferred. My questiona are really: […]
This was just a copy/paste of the existing code: https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/th...
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
This was just a copy/paste of the existing code: https://source.chromium. […]
Kyösti, I was thinking we would fix smp/node.h in a similar fashion. Yes, I don't think we should be #define'ing functions like this. Ideally, we'd have an inline function instead of a forward declaration. But I'd imagine, we can fix all that (including existing arch/ code) up going down the path of providing per-stage semantics.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
Kyösti, I was thinking we would fix smp/node.h in a similar fashion. […]
Did we already not discuss, while <arch/io.h> was added, about placing a copy-pasted <arch/mmio.h> under psp_verstage/include? And reject it?
It's vey much the same thing here, relying on order of -I arguments in CPPFLAGS to keep things working. Then again, there was no existing <smp/spinlock.h> under armv7/ so I would say the file was added in the wrong directory. Easy enough to fix once it is assigned to someone to do.
Aaron; how are issues like these ever going to be addressed, when such workarounds are silently submitted with (IMHO unjustified) expedited reviews.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
Did we already not discuss, while <arch/io.h> was added, about placing a copy-pasted <arch/mmio. […]
I wasn't following the discussion around arch/ that closely. As for a solution should we have a user mode binding under arch/arm or do you think we should just handle these common patterns in the main #includes? I'm thinking the latter as a first pass. I can take that on if you think it's a good starting point.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
I wasn't following the discussion around arch/ that closely. […]
Just want to point out that this is a copy of an existing file in the tree. https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src...
If the 3 +2 reviews needed to submit should be restricted to only fixing breakages in the coreboot master build, maybe the rule should be updated so that everyone else understands that expectation. Currently, that's not stated, so if that's the expectation, nobody else knows it.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
I wasn't following the discussion around arch/ that closely. […]
https://review.coreboot.org/c/coreboot/+/41816/7/src/soc/amd/picasso/psp_ver...
My conclusion of that is platform code under soc/ should not override headerfiles with same names under arch/ and have it depend on order of added CPPFLAGS to work as expected. There is currently no armv7/arch/smp/spinlock.h but maybe there should be.
Seems to me the real problem is <smp/spinlock.h> with CONFIG_SMP test. Maybe the stubs there should be removed and/or replace all <amp/spinlock.h> with <arch/spinlock.h> ?
There is similar CONFIG_SMP case in <smp/node.h> to solve and a missing <arch/smp/atomic.h>. There may be yet more asymmetry that I just don't see yet.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
Just want to point out that this is a copy of an existing file in the tree.
The questions that appeared, only after submission, indicate the file you forked from was alredy borked. And there is also the potential file-name collision (dependency of include path order) that is not desireable.
The quartet AD, FS, JW and KM have authored, reviewed and submitted some 100-200 changes that today enables one to build stages with different ARCH. None of us were added as reviewers.
If the 3 +2 reviews needed to submit should be restricted to only fixing breakages in the coreboot master build, maybe the rule should be updated so that everyone else understands that expectation. Currently, that's not stated, so if that's the expectation, nobody else knows it.
Indeed, and I have requested this for todays meeting agenda. The "3x +2 fast-tracking" guideline wasn't really discussed or argumented much when it first appeared.
https://mail.coreboot.org/pipermail/coreboot/2015-October/080557.html
IMHO the spirit then was it has to be something trivial to fast-track anything, the group with +2 rights back in 2015 may have been more restricted too.
Since this only fixes build with CMOS_POST=y, which no board seems to select by default, what was the motivation for fast-tracking it at all?
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/43310 )
Change subject: soc/amd/picasso: Add dummy spinlock for psp_verstage ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... File src/soc/amd/picasso/psp_verstage/include/arch/smp/spinlock.h:
https://review.coreboot.org/c/coreboot/+/43310/2/src/soc/amd/picasso/psp_ver... PS2, Line 15: #define boot_cpu() 1
Just want to point out that this is a copy of an existing file in the tree. […]
"Fast-track" and "trivial" definitely were two separate categories under which things can be brought in without waiting 24 hours. That said, I reworked that part of the gerrit guidelines in https://review.coreboot.org/c/coreboot/+/43484 for hopefully more clarity. Please take the discussion over there.
-
Aaron Durbin (Code Review) -
Karthik Ramasubramanian (Code Review)
-
Kyösti Mälkki (Code Review)
-
Marshall Dawson (Code Review)
-
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)