Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39133 )
Change subject: mb/kontron: Add Kontron mAL10 COMe module support ......................................................................
Patch Set 53:
(10 comments)
Patch Set 52:
Yeah, no worries. I really meant "take over" not "run over" ^^ Just wanted to let you know that you have more resources at your disposal. We wouldn't want to start the review from scratch. In any case, we'll add our variants on top of this.
Beside the comments here, it would be nice if you could rebase this change (and the reviewed one about carrier dirs) directly on master. Other patches don't look absolutely necessary (e.g. testing SMBus on windows... oO).
Anyway, take your time. I just wanted to make sure this doesn't stall indefinitely :)
I really don't have a lot of time as I have high priority work on another project right now. Sorry if I hold down your research, but I should show this patch to Customer's company afther merged. I've made some fixes and suggest to add this "as it is" to continue working on the Kontron boards together in the next patches. What do you think about it? Thanks.
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... File src/mainboard/kontron/mal10/Kconfig:
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 28: This option sets the type of carrier board to be used with
Done
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 29: kontron mal10
Capitalize accordingly? This help text is meant for human consumption
Done
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 29: come
COMe
Done
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... File src/mainboard/kontron/mal10/ramstage.c:
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 9: size_t
<stddef. […]
Done
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... File src/mainboard/kontron/mal10/romstage.c:
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 3: include <string.h>
not used
Done
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... File src/mainboard/kontron/mal10/variants/mal10/devicetree.cb:
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 5: # Enable VT-d
These comments say nothing useful. […]
Done
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 101: # Set LPC Serial IRQ mode
Not much is gained from this comment either.
Done
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 103: register "uart[0]" = "{ KEMPLD_UART_3F8, 4 }" : register "uart[1]" = "{ KEMPLD_UART_2F8, 3 }"
nit: place these within the corresponding devices?
Done
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 115: register "sensor_mode.local_sensor_enable" = "1" : register "sensor_mode.rtd3" = "RTD_VOLTAGE_MODE" : register "sensor_mode.rtd2" = "RTD_VOLTAGE_MODE" : register "sensor_mode.rtd1" = "RTD_THERMISTOR_MODE"
I think something like this should work: […]
Done
https://review.coreboot.org/c/coreboot/+/39133/48/src/mainboard/kontron/mal1... PS48, Line 127: 0x5F
I've never seen a weather forecast specify temperatures in hex. I wonder why.
Done
-
Maxim Polyakov (Code Review)