Attention is currently required from: Máté Kukri (mkukri), Michael Niewöhner, Felix Held. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/55250 )
Change subject: [WIP] src/superio/smsc: Add support for the SCH5555 ......................................................................
Patch Set 4:
(13 comments)
File src/superio/smsc/sch5555/ramstage.c:
https://review.coreboot.org/c/coreboot/+/55250/comment/0818a4c7_9e266e57 PS4, Line 4: #include <device/pnp.h> : #include <console/console.h> nit: I'd swap these two includes for the sake of alphabetical ordering
https://review.coreboot.org/c/coreboot/+/55250/comment/919403a9_a2100637 PS4, Line 46: struct device *lpci = dev_find_slot_pnp(dev->path.pnp.port, SCH5555_LDN_LPCI); : if (!lpci) { : printk(BIOS_ERR, "SCH5555 LPC interface not present in device tree!\n"); : return; : } I would do this once in `sch5555_set_resources()` and pass around the `lpci` device pointer as parameter. This avoids repeating the code and the error message in logs.
https://review.coreboot.org/c/coreboot/+/55250/comment/a8b52c34_5b7817ef PS4, Line 53: uint8_t bar nit: could be const.
Also, shouldn't we avoid trying to assign a BAR if `ldn_to_bar()` returns 0?
https://review.coreboot.org/c/coreboot/+/55250/comment/ad9e1cb7_7245fd0a PS4, Line 54: uint8_t tmp = pnp_read_config(lpci, bar + 1); : pnp_write_config(lpci, bar + 1, tmp | 0x80); How about:
pnp_unset_and_set_config(lpci, bar + 1, 0, 1 << 7);
https://review.coreboot.org/c/coreboot/+/55250/comment/2fe4daab_8d313a5d PS4, Line 59: pnp_set_logical_device(dev); Any reason to do this? Same for IRQ/DRQ
https://review.coreboot.org/c/coreboot/+/55250/comment/1a77ce8c_f20765e3 PS4, Line 72: pnp_write_config(dev, index, irq); I would add a `pnp_set_logical_device(dev);` call before this write.
https://review.coreboot.org/c/coreboot/+/55250/comment/bb898da7_07985ad3 PS4, Line 85: pnp_write_config(lpci, SCH5555_LPCI_IRQ_CONF + irq, dev->path.pnp.device); I'd parametrise the `SCH5555_LPCI_IRQ_CONF` macro:
#define SCH5555_LPCI_IRQ_CONF(irq) (0x40 + (irq))
The parentheses are necessary to avoid operation order problems.
https://review.coreboot.org/c/coreboot/+/55250/comment/312b9055_be40591f PS4, Line 107: SCH5555_LPCI_DMA_CONF + drq * 2 I'd parametrise the `SCH5555_LPCI_DMA_CONF` macro:
#define SCH5555_LPCI_DMA_CONF(drq) (0x50 + (drq) * 2)
The parentheses are necessary to avoid operation order problems.
https://review.coreboot.org/c/coreboot/+/55250/comment/f53d7597_40806307 PS4, Line 107: dev I'd use `lpci` instead of `dev` here.
https://review.coreboot.org/c/coreboot/+/55250/comment/1189f757_0504eedf PS4, Line 152: SCH5555_LDN_PARPORT How about `SCH5555_LDN_PP` instead? It's shorter and consistent with SCH5545
File src/superio/smsc/sch5555/sch5555.h:
https://review.coreboot.org/c/coreboot/+/55250/comment/3a49d43f_889d231f PS4, Line 9: #define SCH5555_LDN_SELECT 0x07 Is this needed?
https://review.coreboot.org/c/coreboot/+/55250/comment/bd7b2263_78c7463b PS4, Line 26: #define SCH5555_LDN_ACTIVE 0x30 Is this needed?
https://review.coreboot.org/c/coreboot/+/55250/comment/ab1b5750_68eb98c1 PS4, Line 46: the datasheet "the SCH5627P datasheet" ?
-
Angel Pons (Code Review)