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/6928e0b6_75b1aa60?usp... : PS11, Line 35: CONFIG_FSP_STATUS_GLOBAL_RESET
I have a hard time to follow all of your thoughts. I'm not saying anything is wrong, but I also don't see that everything relates to the code, neither the current nor the state after this patch.
I work more with FSP code and FSP evolution than the coreboot community in general, so I have more thoughts that might seem impractical to you unless you've experienced it firsthand. I'm not complaining, though.
For instance:
Therefore, it is important for me to understand that FSP and coreboot are using the same reset numbers without any need for conversion.
Why bring it up? Does this change with this patch?
Yes, it does FSP reset type 9 and 10 was not supported in FSP spec < 2.4. Now with this path, one could still implement those reset types in FSP and coreboot will honor that w/o any complain. But current coreboot code will be able be catch such mismatch and error out.
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.
Where do you see this in the code?
Take a look at https://review.coreboot.org/c/coreboot/+/84356/12/src/soc/intel/apollolake/K...
what if any future platform passes index 10 from SoC code, how will you be sure that 10 reset is actually supported by the current FSP or not?
``` config FSP_STATUS_GLOBAL_RESET_SUFFIX default 10 ```
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.
How? The selectable Kconfigs that are removed by this patch start at `_3`, i.e. no way to select a cold (`_1`) or warm reset (`_2`). Please point to the code (in coreboot history if necessary) where "we used to select" this.
What if I could set the FSP reset type to 1 using this kconfig override from the SoC code? It's unclear whether 1 or 2 are supported by the global reset type. Coreboot won't be able to figure out why FSP is expecting a cold or warm as global reset. Currently, we only support FSP reset types between 3 and 8, so if FSP exits with any other reset type, we halt the boot. What I'm asking for is more granular accountability for FSP reset types, rather than just blindly honoring them in Coreboot.
``` config FSP_STATUS_GLOBAL_RESET_SUFFIX default 1 ```
you can always ignore it as I haven't marked CR-2 (like many code reviewers often do)
Yes, you did not give a -2. But there are other ways to give a change owner or a reviewer a bad time. For instance repeatedly making a lot of wrong or unrelated arguments can sometimes be considered harassment.
I would hold off on commenting here, because what seems wrong to you might be relevant for me to understand if FSP is trying to find a way to support `n` number of reset types and coreboot just simply allows that without understanding the reason behind each reset type/status. A fixed range like 3-8 or 3-10 as per spec is more controllable than just converting any index into a reset type and matching against FSP reset status to say, "coreboot supports this reset type."
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?
I don't think it does. I guess FSP is not the kind of spec that you can take literally and hope it all works.
FSP is a standard spec and we can always give the right feedback. Why to underestimate the FSP spec ?
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.
Seems unnecessary boilerplate if we can instead cover all FSP 2.x versions with a simple abstraction.
Yes, you can convert everything using a simple conversion. But can you explain why FSP is passing different index values per platform? What value is supported according to which spec? Can I pass index value 50 (as per FSP spec 2.3) which is not listed in the FSP spec? What does reset type 50 mean? As I mentioned earlier, I want to maintain a guided range that the FSP spec claims as a valid FSP reset type range, and I want to compare if FSP is sending the reset type according to the range.
Moving that control from FSP driver code to SoC and/or main board and allowing overriding might undermine the purpose of FSP-defined reset type. If FSP claims that any OEM and platform are free to alter that reset type and pass their own index, then I agree that all my feedback is not valid. In that case, allowing SoC to override the FSP reset type is more meaningful.
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.
I don't understand what you are trying to tell us with this. It's roughly what this patch achieves, just indirectly by using the edk headers, isn't it?
this it does. no complain here.
This is what I even wrote in the previous comment as well
``` In August 2022, Intel released FSP spec version 2.4. This spec updates the FSP status code and OEM status code definitions to support both 32-bit and 64-bit interfaces.
The updated spec recommends removing the need for the 32-bit hardcoded macros. (this is my interpretation as I don't see the need to have harcoded 32-bit value)
Instead, it asks that the MAX_BIT macro be used to calculate the OEM status code logic. The only visible change expected to adopt that implementation is that, instead of selecting a direct 32-bit OEM status code, the SoC platform must select a Reset type (ranging from 1-10, where 1 is Cold and 2 is Warm). ```