Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/skylake: Control fixed io decode from device tree ......................................................................
Patch Set 3:
(7 comments)
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG@9 PS2, Line 9: doesn
doesn’t
Done
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG@9 PS2, Line 9: io
IO
Done
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG@9 PS2, Line 9: lpc
LPC
Done
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG@10 PS2, Line 10: io
IO
Done
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG@12 PS2, Line 12: handing
I would rewrite the sentence: […]
Done
https://review.coreboot.org/c/coreboot/+/38748/2/src/soc/intel/common/block/... File src/soc/intel/common/block/include/intelblocks/lpc_lib.h:
https://review.coreboot.org/c/coreboot/+/38748/2/src/soc/intel/common/block/... PS2, Line 75: the current
Um, not really
Why not, this is basically what it does.
The LPC.IOE decides which IO ranges are forwarded to the LPC bus (COMA, COMB, and the fixed ones).
LPC_IOD defines which io ranges are used for COMA, COMB (and the other configurable ranges).
In fact the current naming is a bit misleading in my view. I can change the name to lpc_set_fixed_io_decode of course. At least than the naming is consistent. What do you think.
https://review.coreboot.org/c/coreboot/+/38748/2/src/soc/intel/skylake/bootb... File src/soc/intel/skylake/bootblock/pch.c:
https://review.coreboot.org/c/coreboot/+/38748/2/src/soc/intel/skylake/bootb... PS2, Line 132: config->lpc_ioe
What if I forget to define `lpc_iod` ? […]
That is what I initially did as well but this doesn't really work out well. The issue is that 0 is valid for lpc_iod, it is the default. This means I can't make a difference between a situation where only COMA is set to 2f8 and the case where lpc_iod is omitted.
Given this I think adding the check doesn't add a lot of value. The only check I can think of that adds value is a buildtime check to make sure the COMA and COMB values are not equal when both area's are enabled. I haven't added that as I don't think it is really required and most of the coreboot code doesn't contain this type of configuration checking.