Change in coreboot[master]: soc/intel/common/block/lpc: Add support to allow all UART options
Wim Vervoorn has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/common/block/lpc: Add support to allow all UART options ......................................................................
soc/intel/common/block/lpc: Add support to allow all UART options
The current implementation only allows 0x3f8 for COMA and 0x2f8 for COMB in the LPC decode.
Add the support to allow selection of all options supported by the PCH.
BUG=N/A TEST=build
Change-Id: Iad7bb0e44739e8d656a542c79af7f98a4e9bde69 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/common/block/lpc/Kconfig M src/soc/intel/common/block/lpc/lpc_def.h 2 files changed, 24 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/38748/1
diff --git a/src/soc/intel/common/block/lpc/Kconfig b/src/soc/intel/common/block/lpc/Kconfig index 41e72c4..b95e9be 100644 --- a/src/soc/intel/common/block/lpc/Kconfig +++ b/src/soc/intel/common/block/lpc/Kconfig @@ -11,3 +11,25 @@ help By default COMA range to LPC is enable. COMB range to LPC is optional and should select based on platform dedicated selection. + +config SOC_INTEL_COMMON_BLOCK_LPC_COMA_UART + int + prompt "Index for COMA UART" + depends on SOC_INTEL_COMMON_BLOCK_LPC && DRIVERS_UART_8250IO + default 0 + range 0 7 + help + Select an I/O port for COMA (Used to open LPC IO window) + 0 = 0x3f8, 1 = 0x2f8, 2 = 0x220, 3 = 0x228, + 4 = 0x238, 5 = 0x2e8, 6 = 0x338, 7 = 0x3e8 + +config SOC_INTEL_COMMON_BLOCK_LPC_COMB_UART + int + prompt "Index for COMB UART" if DRIVERS_UART_8250IO + depends on SOC_INTEL_COMMON_BLOCK_LPC && DRIVERS_UART_8250IO && SOC_INTEL_COMMON_BLOCK_LPC_COMB_ENABLE + default 1 + range 0 7 + help + Select an I/O port for COMB (Used to open LPC IO window) + 0 = 0x3f8, 1 = 0x2f8, 2 = 0x220, 3 = 0x228, + 4 = 0x238, 5 = 0x2e8, 6 = 0x338, 7 = 0x3e8 diff --git a/src/soc/intel/common/block/lpc/lpc_def.h b/src/soc/intel/common/block/lpc/lpc_def.h index 9a72580..7af235f 100644 --- a/src/soc/intel/common/block/lpc/lpc_def.h +++ b/src/soc/intel/common/block/lpc/lpc_def.h @@ -21,8 +21,8 @@ #define LPC_SCNT_EN (1 << 7) #define LPC_SCNT_MODE (1 << 6) #define LPC_IO_DECODE 0x80 -#define LPC_IOD_COMA_RANGE (0 << 0) /* 0x3F8 - 0x3FF COMA*/ -#define LPC_IOD_COMB_RANGE (1 << 4) /* 0x2F8 - 0x2FF COMB*/ +#define LPC_IOD_COMA_RANGE (CONFIG_SOC_INTEL_COMMON_BLOCK_LPC_COMA_UART << 0) +#define LPC_IOD_COMB_RANGE (CONFIG_SOC_INTEL_COMMON_BLOCK_LPC_COMB_UART << 4) /* Use IO_<peripheral>_<IO port> style macros defined in lpc_lib.h * to enable decoding of I/O locations for a peripheral. */ #define LPC_IO_ENABLES 0x82
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/common/block/lpc: Add support to allow all UART options ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 17: prompt "Index for COMA UART" Why the prompt? Doesn't this depend on devicetree or static hardware settings?
If there is no reason for a prompt, such options should be part of the devicetree (or just be derived from the existing devicetree information at runtime).
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 18: DRIVERS_UART_8250IO I know it's probably derived from line 8, but I don't understand it... DRIVERS_UART_8250IO is about the driver coreboot uses for its console.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/common/block/lpc: Add support to allow all UART options ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 17: prompt "Index for COMA UART"
Why the prompt? Doesn't this depend on devicetree or static hardware settings? […]
Right now this information is not in the device tree so we would have to add it. Right now only the generic ranges can be defined there. The code as it is always opens the 3f8 window and opens the 2f8 window if SOC_INTEL_COMMON_BLOCK_LPC_COMB_UART is defined. Which obviously isn't a very good idea.
What I did here was a quick solution for this issue that doesn't require changes for existing boards. If we need to add it to the device tree I am wondering what we should add, just the com ports? All possibilities in the register? Or would it be better to create an interface that can be used by code actually initializing a port. This would then request a range to be opened and the lpc handler can then decide how to deal with it. I guess this would be nice but a lot of work.
What is your suggestion here?
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 18: DRIVERS_UART_8250IO
I know it's probably derived from line 8, but I don't understand it... […]
TYou only need to open up the com port windows if an IO based UART is used. The MMIO based uarts don't require this. This is also the reason why it is used in line 8.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/common/block/lpc: Add support to allow all UART options ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 17: prompt "Index for COMA UART"
Right now this information is not in the device tree so we would have to add it. Right now only the generic ranges can be defined there. The code as it is always opens the 3f8 window and opens the 2f8 window if SOC_INTEL_COMMON_BLOCK_LPC_COMB_UART is defined. Which obviously isn't a very good idea.
Not obvious to me, please elaborate. It's not like the user could plug an external UART chip on the LPC, is it? Is there a mainboard that uses this common code and has an i/o based UART at different addresses?
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 18: DRIVERS_UART_8250IO
TYou only need to open up the com port windows if an IO based UART is used. […]
I know the technical background, but not what it has to do with coreboot's console. DRIVERS_UART_8250IO is a software feature and not about the hardware.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/common/block/lpc: Add support to allow all UART options ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 18: DRIVERS_UART_8250IO
I know the technical background, but not what it has to do with coreboot's […]
You are right about this. Unfortunately this is not how it's used in severla places so this would need to be updated. Which isn't a big problem.
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/common/block/lpc: Add support to allow all UART options ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 17: prompt "Index for COMA UART"
Right now this information is not in the device tree so we would have to add it. […]
I think this depends on the way you look at the code. In my view, the generic support code included in the chipset directories (like north/south bride soc, etc) should be as generic as possible and should not make assumptions on the use by mainboards. Because this limits the usability of the code and encourages board implementors to implement the same functionality in a non-standard way.
In this case, it would mean that this code should support all possible options in the LPC bridge that we want to support. Right now only the 2 com port ranges are handled. The FDD and LPT range are not. The FDD makes sense I can't imagine somebody is still using this. There may still boards that want to support LPT I guess. Another one is the IO enables register. I had a look at this and the handling is different for each chipset and there is no way to configure on the board level other than by changing the chipset code.
The IO enables register offers 6 options: 2E, 4E, KBC, EC, LGE and HGA. If I look at the implementation for cnl, icl, skl and tgl I get the following results: cnl enables all but HGA. icl and tgl all but 4E and HGA and skl only enables 2E, KBC and EC.
If we want to fully configure the lpc forwarding from the device tree. I would suggest to add a 16 lpc_io_enables item that overrides the chipset default and a 16 bit lpc_io_decode item that allows control over the io decode ranges.
The SOC_INTEL_COMMON_BLOCK_LPC_COMB_UART item can be dropped in this case.
The advantage of the above is that addresses that aren't used are not forwarded to the lpc bus.
To answer your last question, yes we've seen cases where the 2nd UART couldn't be configured to the 2f8 default. Reasons have been custom hardware occupying the same address or legacy software requiring another address.
It's a long story but I hope this clarifies my earlier remarks.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/common/block/lpc: Add support to allow all UART options ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 17: prompt "Index for COMA UART"
To answer your last question, yes we've seen cases where the 2nd UART couldn't be configured to the 2f8 default. Reasons have been custom hardware occupying the same address or legacy software requiring another address.
I take that as a `no`, as I asked specifically for a case that uses this code.
It's a long story but I hope this clarifies my earlier remarks.
Yes, I guess. It seems you want to add generic, but unused (at the moment) functionality. IMO, a nice intent, but nothing will prevent it from breaking before anybody makes use of it.
IMHO, we should only support what in-tree boards need. If we wanted to support every possible case of the silicon, we'd be looking forward to a ten times bigger code base.
If we want to fully configure the lpc forwarding from the device tree. I would suggest to add a 16 lpc_io_enables item that overrides the chipset default and a 16 bit lpc_io_decode item that allows control over the io decode ranges.
All necessary information is already (supposed to be) in the device tree. All we need is to fill the registers with it. Have a look at soc/intel/common/block/ lpc/lpc_lib.c, lpc_open_pmio_window(). This code already handles the case for non-standard i/o port ranges. All what's missing is a list of the port ranges supported by the lp_io_decode/_enables registers, to handle these specifically.
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/common/block/lpc: Add support to allow all UART options ......................................................................
Patch Set 1: Code-Review+2
Wim Vervoorn has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/common/block/lpc: Add support to allow all UART options ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 17: prompt "Index for COMA UART"
To answer your last question, yes we've seen cases where the 2nd UART couldn't be configured to th […]
I am sorry for the late reply, I have been occupied in another project and embedded world.
I know that the common block lpc code is handling the generic ranges. The issue is with the other ones. What I am proposing in the lines above is to add items for the lpc_io_ranges and enables register. The all that is missing part of your remark.
Hello build bot (Jenkins), Frans Hendriks, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38748
to look at the new patch set (#2).
Change subject: soc/intel/skylake: Control fixed io decode from device tree ......................................................................
soc/intel/skylake: Control fixed io decode from device tree
The current implementation doesn allow custom values for the lpc io decodes and io enables.
Add the lpc_ioe and lpc_iod values. When specified the handing overrides the current handling for COMA and COMB.
BUG=N/A TEST=tested on facebook monolith
Change-Id: Iad7bb0e44739e8d656a542c79af7f98a4e9bde69 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/common/block/include/intelblocks/lpc_lib.h M src/soc/intel/common/block/lpc/lpc_lib.c M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip.h 4 files changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/38748/2
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 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/Kconfig:
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 17: prompt "Index for COMA UART"
I am sorry for the late reply, I have been occupied in another project and embedded world. […]
Done
https://review.coreboot.org/c/coreboot/+/38748/1/src/soc/intel/common/block/... PS1, Line 18: DRIVERS_UART_8250IO
You are right about this. […]
Done
Paul Menzel 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 2:
(2 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
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG@12 PS2, Line 12: handing handling?
Angel Pons 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 2: Code-Review+1
(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: lpc LPC
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG@9 PS2, Line 9: io IO
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG@10 PS2, Line 10: io IO
https://review.coreboot.org/c/coreboot/+/38748/2//COMMIT_MSG@12 PS2, Line 12: handing
handling?
I would rewrite the sentence:
If they are not zero, they will be used instead of the current handling for COMA and COMB.
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
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` ?
I would check for both.
https://review.coreboot.org/c/coreboot/+/38748/2/src/soc/intel/skylake/chip.... File src/soc/intel/skylake/chip.h:
https://review.coreboot.org/c/coreboot/+/38748/2/src/soc/intel/skylake/chip.... PS2, Line 82: lpc LPC
Hello build bot (Jenkins), Nico Huber, Frans Hendriks, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38748
to look at the new patch set (#3).
Change subject: soc/intel/skylake: Control fixed io decode from device tree ......................................................................
soc/intel/skylake: Control fixed io decode from device tree
The current implementation doesn't allow custom values for the LPC IO decodes and IO enables.
Add the lpc_ioe and lpc_iod values. If they are not zero, they will be used instead of the current handling for COMA and COMB.
BUG=N/A TEST=tested on facebook monolith
Change-Id: Iad7bb0e44739e8d656a542c79af7f98a4e9bde69 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/common/block/include/intelblocks/lpc_lib.h M src/soc/intel/common/block/lpc/lpc_lib.c M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip.h 4 files changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/38748/3
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.
Patrick Rudolph 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: Code-Review+1
Hello build bot (Jenkins), Nico Huber, Frans Hendriks, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38748
to look at the new patch set (#4).
Change subject: soc/intel/skylake: Control fixed io decode from device tree ......................................................................
soc/intel/skylake: Control fixed io decode from device tree
The current implementation doesn't allow custom values for the LPC IO decodes and IO enables.
Add the lpc_ioe and lpc_iod values. If they are not zero, they will be used instead of the current handling for COMA and COMB.
BUG=N/A TEST=tested on facebook monolith
Change-Id: Iad7bb0e44739e8d656a542c79af7f98a4e9bde69 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/common/block/include/intelblocks/lpc_lib.h M src/soc/intel/common/block/lpc/lpc_lib.c M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip.h 4 files changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/38748/4
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 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/38748/2/src/soc/intel/skylake/chip.... File src/soc/intel/skylake/chip.h:
https://review.coreboot.org/c/coreboot/+/38748/2/src/soc/intel/skylake/chip.... PS2, Line 82: lpc
LPC
Done
Angel Pons 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 4: Code-Review+1
(5 comments)
https://review.coreboot.org/c/coreboot/+/38748/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38748/4//COMMIT_MSG@7 PS4, Line 7: device tree devicetree
https://review.coreboot.org/c/coreboot/+/38748/4//COMMIT_MSG@7 PS4, Line 7: io IO
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
Why not, this is basically what it does. […]
Well, `lpc_set_fixed_io_ranges` sets the decode ranges according to its parameters, so it does not set "the current" decode ranges (rather, it sets new ones).
Maybe say: "Configure IO decode ranges"
https://review.coreboot.org/c/coreboot/+/38748/4/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/38748/4/src/soc/intel/common/block/... PS4, Line 45: lpc_set_fixed_io_ranges Can we rename this to `lpc_set_fixed_io_decode` for consistency? It touches the same register as `lpc_get_fixed_io_decode` above.
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
That is what I initially did as well but this doesn't really work out well. […]
Ack
Hello build bot (Jenkins), Nico Huber, Frans Hendriks, Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38748
to look at the new patch set (#5).
Change subject: soc/intel/skylake: Control fixed IO decode from devicetree ......................................................................
soc/intel/skylake: Control fixed IO decode from devicetree
The current implementation doesn't allow custom values for the LPC IO decodes and IO enables.
Add the lpc_ioe and lpc_iod values. If they are not zero, they will be used instead of the current handling for COMA and COMB.
BUG=N/A TEST=tested on facebook monolith
Change-Id: Iad7bb0e44739e8d656a542c79af7f98a4e9bde69 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com --- M src/soc/intel/common/block/include/intelblocks/lpc_lib.h M src/soc/intel/common/block/lpc/lpc_lib.c M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip.h 4 files changed, 27 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/48/38748/5
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 devicetree ......................................................................
Patch Set 5:
(4 comments)
https://review.coreboot.org/c/coreboot/+/38748/4//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/38748/4//COMMIT_MSG@7 PS4, Line 7: io
IO
Done
https://review.coreboot.org/c/coreboot/+/38748/4//COMMIT_MSG@7 PS4, Line 7: device tree
devicetree
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
Well, `lpc_set_fixed_io_ranges` sets the decode ranges according to its parameters, so it does not s […]
Done
https://review.coreboot.org/c/coreboot/+/38748/4/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc_lib.c:
https://review.coreboot.org/c/coreboot/+/38748/4/src/soc/intel/common/block/... PS4, Line 45: lpc_set_fixed_io_ranges
Can we rename this to `lpc_set_fixed_io_decode` for consistency? It touches the same register as `lp […]
Done
Frans Hendriks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/skylake: Control fixed IO decode from devicetree ......................................................................
Patch Set 5: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/skylake: Control fixed IO decode from devicetree ......................................................................
soc/intel/skylake: Control fixed IO decode from devicetree
The current implementation doesn't allow custom values for the LPC IO decodes and IO enables.
Add the lpc_ioe and lpc_iod values. If they are not zero, they will be used instead of the current handling for COMA and COMB.
BUG=N/A TEST=tested on facebook monolith
Change-Id: Iad7bb0e44739e8d656a542c79af7f98a4e9bde69 Signed-off-by: Wim Vervoorn wvervoorn@eltan.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38748 Reviewed-by: Frans Hendriks fhendriks@eltan.com Reviewed-by: Angel Pons th3fanbus@gmail.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/soc/intel/common/block/include/intelblocks/lpc_lib.h M src/soc/intel/common/block/lpc/lpc_lib.c M src/soc/intel/skylake/bootblock/pch.c M src/soc/intel/skylake/chip.h 4 files changed, 27 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Frans Hendriks: Looks good to me, approved Angel Pons: Looks good to me, but someone else must approve
diff --git a/src/soc/intel/common/block/include/intelblocks/lpc_lib.h b/src/soc/intel/common/block/include/intelblocks/lpc_lib.h index cf6d8e9..25a7370 100644 --- a/src/soc/intel/common/block/include/intelblocks/lpc_lib.h +++ b/src/soc/intel/common/block/include/intelblocks/lpc_lib.h @@ -72,6 +72,8 @@ uint16_t lpc_enable_fixed_io_ranges(uint16_t io_enables); /* Return the current decode settings */ uint16_t lpc_get_fixed_io_decode(void); +/* Set the current decode ranges */ +uint16_t lpc_set_fixed_io_ranges(uint16_t io_ranges, uint16_t mask); /* Open a generic IO window to the LPC bus. Four windows are available. */ void lpc_open_pmio_window(uint16_t base, uint16_t size); /* Close all generic IO windows to the LPC bus. */ diff --git a/src/soc/intel/common/block/lpc/lpc_lib.c b/src/soc/intel/common/block/lpc/lpc_lib.c index 5b30a81..8edbd2e 100644 --- a/src/soc/intel/common/block/lpc/lpc_lib.c +++ b/src/soc/intel/common/block/lpc/lpc_lib.c @@ -42,6 +42,17 @@ return pci_read_config16(PCH_DEV_LPC, LPC_IO_DECODE); }
+uint16_t lpc_set_fixed_io_ranges(uint16_t io_ranges, uint16_t mask) +{ + uint16_t reg_io_ranges; + + reg_io_ranges = lpc_get_fixed_io_decode() & ~mask; + io_ranges |= reg_io_ranges & mask; + pci_write_config16(PCH_DEV_LPC, LPC_IO_DECODE, io_ranges); + + return io_ranges; +} + /* * Find the first unused IO window. * Returns -1 if not found, 0 for reg 0x84, 1 for reg 0x88 ... diff --git a/src/soc/intel/skylake/bootblock/pch.c b/src/soc/intel/skylake/bootblock/pch.c index d5d3aed..b9a5633 100644 --- a/src/soc/intel/skylake/bootblock/pch.c +++ b/src/soc/intel/skylake/bootblock/pch.c @@ -127,9 +127,16 @@ uint16_t io_enables = LPC_IOE_SUPERIO_2E_2F | LPC_IOE_KBC_60_64 | LPC_IOE_EC_62_66;
- /* IO Decode Range */ - if (CONFIG(DRIVERS_UART_8250IO)) - lpc_io_setup_comm_a_b(); + const config_t *config = config_of_soc(); + + if (config->lpc_ioe) { + io_enables = config->lpc_ioe & 0x3f0f; + lpc_set_fixed_io_ranges(config->lpc_iod, 0x1377); + } else { + /* IO Decode Range */ + if (CONFIG(DRIVERS_UART_8250IO)) + lpc_io_setup_comm_a_b(); + }
/* IO Decode Enable */ if (pch_check_decode_enable() == 0) { diff --git a/src/soc/intel/skylake/chip.h b/src/soc/intel/skylake/chip.h index 2c3d3a5..69b818b 100644 --- a/src/soc/intel/skylake/chip.h +++ b/src/soc/intel/skylake/chip.h @@ -79,6 +79,10 @@ uint8_t gpe0_dw1; /* GPE0_63_32 STS/EN */ uint8_t gpe0_dw2; /* GPE0_95_64 STS/EN */
+ /* LPC fixed enables and ranges */ + uint16_t lpc_iod; + uint16_t lpc_ioe; + /* Generic IO decode ranges */ uint32_t gen1_dec; uint32_t gen2_dec;
9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38748 )
Change subject: soc/intel/skylake: Control fixed IO decode from devicetree ......................................................................
Patch Set 6:
Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3 Emulation targets: EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/1415 EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1414 EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/1413
Please note: This test is under development and might not be accurate at all!
-
9elements QA (Code Review) -
Angel Pons (Code Review)
-
Frans Hendriks (Code Review)
-
Nico Huber (Code Review)
-
Patrick Georgi (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Wim Vervoorn (Code Review)