Subrata Banik has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/32765
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__
This patch removes multiple(defined(__PRE_RAM__) || ENV_SMM || ENV_POSTCAR) checks to enable __SIMPLE_DEVICE__.
Change-Id: Ide61ec2c908758ba96c61ca9ed7278e7d11d1ee5 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/include/rules.h 1 file changed, 4 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/32765/1
diff --git a/src/include/rules.h b/src/include/rules.h index ce968f0..1effe3f 100644 --- a/src/include/rules.h +++ b/src/include/rules.h @@ -262,17 +262,16 @@ #define ENV_PAYLOAD_LOADER ENV_RAMSTAGE
/** - * For pre-DRAM stages and post-CAR always build with simple device model, ie. - * PCI, PNP and CPU functions operate without use of devicetree. The reason - * post-CAR utilizes __SIMPLE_DEVICE__ is for simplicity. Currently there's - * no known requirement that devicetree would be needed during that stage. + * !ENV_PAYLOAD_LOADER enable stages always build with simple device model, + * ie. PCI, PNP and CPU functions operate without use of devicetree. The reason + * post-CAR utilizes __SIMPLE_DEVICE__ is for simplicity. * * For ramstage individual source file may define __SIMPLE_DEVICE__ * before including any header files to force that particular source * be built with simple device model. */
-#if (defined(__PRE_RAM__) || ENV_SMM || ENV_POSTCAR) +#if !ENV_PAYLOAD_LOADER #define __SIMPLE_DEVICE__ #endif
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32765 )
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/32765/2/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/#/c/32765/2/src/include/rules.h@266 PS2, Line 266: The reason : * post-CAR utilizes __SIMPLE_DEVICE__ is for simplicity post-CAR utilizes __SIMPLE_DEVICE__ only if it is not a payload loader stage.
Hello Aaron Durbin, ron minnich, Julius Werner, build bot (Jenkins), Furquan Shaikh, Patrick Georgi,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/32765
to look at the new patch set (#3).
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__
This patch removes multiple(defined(__PRE_RAM__) || ENV_SMM || ENV_POSTCAR) checks to enable __SIMPLE_DEVICE__.
Change-Id: Ide61ec2c908758ba96c61ca9ed7278e7d11d1ee5 Signed-off-by: Subrata Banik subrata.banik@intel.com --- M src/include/rules.h 1 file changed, 4 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/65/32765/3
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32765 )
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/32765/2/src/include/rules.h File src/include/rules.h:
https://review.coreboot.org/#/c/32765/2/src/include/rules.h@266 PS2, Line 266: The reason : * post-CAR utilizes __SIMPLE_DEVICE__ is for simplicity
post-CAR utilizes __SIMPLE_DEVICE__ only if it is not a payload loader stage.
Done
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32765 )
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Patch Set 3: Code-Review-1
I'd rather have __SIMPLE_DEVICE__ completely removed. Unfortunately the work that would allow this (to always use struct device *) in stages where devicetree is present, has been stalled as the related util/sconfig changes are not receiving any reviews.
IMO the proposed changes obfuscates what __SIMPLE_DEVICE__ is/was for since there is no relation to payload loading whatsoever.
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32765 )
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Patch Set 3:
I'd rather have __SIMPLE_DEVICE__ completely removed. Unfortunately the work that would allow this (to always use struct device *) in stages where devicetree is present, has been stalled as the related util/sconfig changes are not receiving any reviews.
IMO the proposed changes obfuscates what __SIMPLE_DEVICE__ is/was for since there is no relation to payload loading whatsoever.
As i understand the use of __SIMPLE_DEVICE__ is to differentiate between Pre DRAM stage and post DRAM stage. So i believe we might still need __SIMPLE_DEVICE__ definition with this approach.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32765 )
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Patch Set 3:
Patch Set 3:
I'd rather have __SIMPLE_DEVICE__ completely removed. Unfortunately the work that would allow this (to always use struct device *) in stages where devicetree is present, has been stalled as the related util/sconfig changes are not receiving any reviews.
IMO the proposed changes obfuscates what __SIMPLE_DEVICE__ is/was for since there is no relation to payload loading whatsoever.
As i understand the use of __SIMPLE_DEVICE__ is to differentiate between Pre DRAM stage and post DRAM stage. So i believe we might still need __SIMPLE_DEVICE__ definition with this approach.
Not quite right. The only reason it was introduced is to solve the name collision we have with PCI config accessors. We needed a way to have the same source code file built for different stages; but different stages use different prototypes for functions like pci_config_read/write().
With the PCI config accessors now inlined, there is no perfomance penalty if we move romstage away from passing pci_devfn_t to struct device *. As matter of fact, for code sections that move data from devicetree.cb to hardware, latter probably creates more tighter assembly output.
https://review.coreboot.org/q/topic:devicetree
Subrata Banik has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32765 )
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Patch Set 3:
Patch Set 3:
I'd rather have __SIMPLE_DEVICE__ completely removed.
Unfortunately
the work that would allow this (to always use struct device *)
in
stages where devicetree is present, has been stalled as the
related
util/sconfig changes are not receiving any reviews.
IMO the proposed changes obfuscates what __SIMPLE_DEVICE__
is/was
for since there is no relation to payload loading whatsoever.
As i understand the use of __SIMPLE_DEVICE__ is to differentiate
between Pre DRAM stage and post DRAM stage. So i believe we might still need __SIMPLE_DEVICE__ definition with this approach.
Not quite right. The only reason it was introduced is to solve the name collision we have with PCI config accessors. We needed a way to have the same source code file built for different stages; but different stages use different prototypes for functions like pci_config_read/write().
[Subrata] Wondering why that might have designed at first place. Might be to avoid use of complex pointer in those pre_ram stages ?
With the PCI config accessors now inlined, there is no perfomance penalty if we move romstage away from passing pci_devfn_t to struct device *. As matter of fact, for code sections that move data from devicetree.cb to hardware, latter probably creates more tighter assembly output.
[Subrata] thanks for your CL, we can get rid of __SIMPLE_DEVICE__ once your topic branch get landed completely. Do you have an ETA when you think it can be done.
For now we want Coreboot_lite implementation being independent so we can speed up.
Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32765 )
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Patch Set 4:
Patch Set 3:
Not quite right. The only reason it was introduced is to solve the name collision we have with PCI config accessors. We needed a way to have the same source code file built for different stages; but different stages use different prototypes for functions like pci_config_read/write().
[Subrata] Wondering why that might have designed at first place. Might be to avoid use of complex pointer in those pre_ram stages ?
Yep, remainer of the days before cache-as-ram, when romstage was built with romcc that could not deal with structs or pointers at all. Also romstage does not have a "complete" devicetree but that part can be sorted out.
With the PCI config accessors now inlined, there is no perfomance penalty if we move romstage away from passing pci_devfn_t to struct device *. As matter of fact, for code sections that move data from devicetree.cb to hardware, latter probably creates more tighter assembly output.
[Subrata] thanks for your CL, we can get rid of __SIMPLE_DEVICE__ once your topic branch get landed completely. Do you have an ETA when you think it can be done.
No ETA; nobody is reviewing my commits on that topic or other ones either so everything is piled up.
For now we want Coreboot_lite implementation being independent so we can speed up.
Well I think you can just skip this particular commit, I don't see why you would need to change anything connected to __SIMPLE_DEVICE__ for that.
Subrata Banik has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/32765 )
Change subject: Use !ENV_PAYLOAD_LOADER to select __SIMPLE_DEVICE__ ......................................................................
Abandoned
parking for now, will see if we need this later
-
Furquan Shaikh (Code Review) -
Kyösti Mälkki (Code Review)
-
Subrata Banik (Code Review)