[M] Change in coreboot[main]: drivers/intel/fsp2_0: Simplify FSP global reset definition
Attention is currently required from: Jérémy Compostella.
Subrata Banik has posted comments on this change by Jérémy Compostella. ( https://review.coreboot.org/c/coreboot/+/84356?usp=email )
Change subject: drivers/intel/fsp2_0: Simplify FSP global reset definition ......................................................................
Patch Set 12:
(1 comment)
File src/soc/intel/common/fsp_reset.c:
https://review.coreboot.org/c/coreboot/+/84356/comment/4e0c9533_b9fb26c1?usp... : PS11, Line 35: CONFIG_FSP_STATUS_GLOBAL_RESET
The updated spec recommends removing the need for the 32-bit hardcoded macros.
Unfair as it does not. FSP 2.0 stated the use of highest bit and provided for the values for 32-bit only because FSP 2.0 is limited to 32-bits.
If that's the case, why did Intel publish the reset type hardcoded macro between 0x40000001-0x40000008 in the FSP 2.3 spec? Why didn't they follow the same calculation they introduced in FSP 2.0 through FSP 2.3 for FSP 2.4?
Subrata, what you reference is just a listing. A listing that tells you what values were chosen, not why. Jeremy has explained multiple times that they *did* use the same calculation (as the spec states in the paragraphs right before the listing). So it's completely futile to ask why they didn't. If you don't have the time to fully follow the spec, that's ok, but then you shouldn't argue.
As a reviewer of the code, you are always welcome to share your thoughts. I have shared my philosophy about this CL and why I am not in favor of passing the index value for the reset type. Instead, I am asking for a complete reset type.
I have concerns about the CL commit message and how it articulates the problem. There is nothing wrong or bad with the previous implementation. We just need to modify the existing modification, which is very 32-bit focused, to add a 64-bit FSP reset type. Hence, I have asked a few things:
What is the exact FSP reset value for 64-bit? Can we explicitly mention the value rather than using a conversion? Why have I asked for it? The reset type listed in the Intel FSP spec is an OEM-specific reset type, and nothing in the FSP spec can force OEMs to follow it. OEMs are free to modify the bit fields to share their own reset type numbers. For example, in the IBV BIOS code (if you have access), these reset types are implemented differently. Therefore, it is important for me to understand that FSP and coreboot are using the same reset numbers without any need for conversion. Ideally, the conversion logic is for EDK2 vendors and not required for coreboot. Coreboot just needs to understand the FSP-requested reset type and honor it (if supported). In the past, without these implementations, I have seen cases where FSP passed a reset type, and coreboot was not aware of how to handle that reset type. I wonder why we are having so many different discussions.
What are we losing when we drop the Kconfig reset type and use the index value and a wrapper code? We are losing control. How?
FSP can implement more reset types that coreboot won't be able to understand. For example, the FSP spec says reset types 1-8 are allowed in the FSP 2.3 spec. With the introduction of the index value in the SOC code, we often miss understanding the meaning of the reset type when someone is passing 3, 5, or 10 (where 10 might be something outside the FSP 2.3 spec). With the current implementation, if FSP sends a reset type as 0x4000000a, coreboot will be unable to match this reset type and fail (easy to catch). However, by passing the index, we hardly care about what 10 is here because the index override will be inside the SOC code, and all FSP driver maintainers won't be able to review those codes. Finally, if FSP can pass 0x4000000a as a reset type, coreboot won't complain because it will find a match using the FSP wrapper that converts any index into the reset type value and issues a global reset. In this case, the behavior is different without this CL (die due to unmatched FSP reset type) and with this CL (issue global reset).
Earlier, if the reset type was warm or cold, we used to select the respective Kconfig from the SoC code. Therefore, during code review, we understood that this platform expects a cold reset while FSP asked to perform a global reset. Now, with the reset type passed as index value 1, we won't be able to understand what 1 means unless someone reads the spec.
Finally, coming to answering your question about whether I have time to read the FSP spec, please leave that to me. If you think my feedback is not valuable, you can always ignore it as I haven't marked CR-2 (like many code reviewers often do). I'm more specific here to understand that coreboot and FSP use the same nomenclature rather than getting hijacked by FSP, where we started using FSP-recommended macros and stuff without asking the right question about how/what benefit I will get with this new code. Are we losing anything? I guess Werner asked a few valuable questions:
Earlier, the default value was 0xffff when we didn't choose a reset type. Now, what is the default behavior? What if we wish to not honor the FSP reset type? Can we do that in coreboot?
Earlier, we had a kconfig to specify the cold reset type. Now, we are hardcoding a value which is hard to understand what that means.
can you please show me if there is any section that says on 32-bit FSP the OEM status code should be a 32-bit width value?
The spec states, "The range of status code that has the highest bit clear and the next to highest bit set are reserved for use by OEMs."
Hence, I'm more specific on the listing that the below-listed value is all 32-bit. Therefore, we should use the exact value in the coreboot to check the matched FSP reset type.
Similarly, I have requested if we can specify the direct 64-bit value.
If we can explicitly use the `MAX_BIT` definition associated with a FSP supported architecture, then converting the FSP reset type from index is generic. We can avoid having specific Kconfig that lists the hardcoded 32-bit value.
-
Subrata Banik (Code Review)