Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/19468 )
Change subject: intel/common/block/i2c: Add common block for I2C ......................................................................
Patch Set 6:
(16 comments)
https://review.coreboot.org/#/c/19468/6/src/soc/intel/common/block/i2c/i2c.c File src/soc/intel/common/block/i2c/i2c.c:
PS6, Line 32: if (devfn >= 0) { : /* devfn -> dev */ : dev = dev_find_slot(0, devfn); : if (dev) { : /* dev -> bar0 */ : res = find_resource(dev, PCI_BASE_ADDRESS_0); : if (res) : return res->base; : } : } if (devfn < 0) return (uintptr_t)NULL;
dev = dev_find_slot(0, devfn); if (!dev) return (uintptr_t)NULL;
res = find_resource(dev, PCI_BASE_ADDRESS_0); if (res) return res->base;
return (uintptr_t)NULL;
You can avoid multiple levels of nesting blocks.
PS6, Line 82: The device should already be enabled and out of reset, Should this driver take the controller out of reset and not rely on caller doing anything for it?
https://review.coreboot.org/#/c/19468/6/src/soc/intel/common/block/i2c/i2c_e... File src/soc/intel/common/block/i2c/i2c_early.c:
PS6, Line 52: { space before {
PS6, Line 53: printk(BIOS_ERR, "I2C%u not enabled for early init\n", bus); Do we want to print this? It could be intentional and not an error.
https://review.coreboot.org/#/c/19468/6/src/soc/intel/common/block/i2c/lpss_... File src/soc/intel/common/block/i2c/lpss_i2c.c:
Line 477: }
Why is this dropped?
It was moved to intel/common/block/i2c/i2c.c
Line 729: }
Why did this get dropped?
Moved to intel/common/block/i2c/i2c.c
PS6, Line 447: int
Why was this exposed globally?
I think because this is called from lpss_i2c_acpi_fill_ssdt which is now moved to a different file.
PS6, Line 592: int lpss_i2c_bus_to_dev(int bus) : { : return i2c_soc_bus_to_devfn(bus); : } : : int lpss_i2c_dev_to_bus(struct device *dev) : { : return i2c_soc_devfn_to_bus(dev->path.pci.devfn); : } Why not just call i2c_soc_* functions instead of having a function call X->lpss_i2c_bus_to_dev->i2c_soc_bus_to_devfn? I think we can simply have X->i2c_soc_bus_to_devfn.
PS6, Line 602: __attribute__((weak)) const struct lpss_i2c_bus_config *i2c_get_soc_cfg(void){ : printk(BIOS_ERR, "SoC does not have I2C configuration!!\n"); : return NULL; : } I think instead of having a weak function here, we would want the compilation to fail if SoC does not provide the config?
https://review.coreboot.org/#/c/19468/6/src/soc/intel/common/block/i2c/lpss_... File src/soc/intel/common/block/i2c/lpss_i2c.h:
Line 1: /* This file has been moved. All boards including this file need to be updated.
PS6, Line 91: /* Frequency represented as ticks per ns. Can also be used to calculate : * the number of ticks to meet a time target or the period. */ : struct freq { : uint32_t ticks; : uint32_t ns; : }; : : /* Control register definitions */ : enum { : CONTROL_MASTER_MODE = (1 << 0), : CONTROL_SPEED_SS = (1 << 1), : CONTROL_SPEED_FS = (1 << 2), : CONTROL_SPEED_HS = (3 << 1), : CONTROL_SPEED_MASK = (3 << 1), : CONTROL_10BIT_SLAVE = (1 << 3), : CONTROL_10BIT_MASTER = (1 << 4), : CONTROL_RESTART_ENABLE = (1 << 5), : CONTROL_SLAVE_DISABLE = (1 << 6), : }; : : /* Command/Data register definitions */ : enum { : CMD_DATA_CMD = (1 << 8), : CMD_DATA_STOP = (1 << 9), : }; : : /* Status register definitions */ : enum { : STATUS_ACTIVITY = (1 << 0), : STATUS_TX_FIFO_NOT_FULL = (1 << 1), : STATUS_TX_FIFO_EMPTY = (1 << 2), : STATUS_RX_FIFO_NOT_EMPTY = (1 << 3), : STATUS_RX_FIFO_FULL = (1 << 4), : STATUS_MASTER_ACTIVITY = (1 << 5), : STATUS_SLAVE_ACTIVITY = (1 << 6), : }; : : /* Enable register definitions */ : enum { : ENABLE_CONTROLLER = (1 << 0), : }; : : /* Interrupt status register definitions */ : enum { : INTR_STAT_RX_UNDER = (1 << 0), : INTR_STAT_RX_OVER = (1 << 1), : INTR_STAT_RX_FULL = (1 << 2), : INTR_STAT_TX_OVER = (1 << 3), : INTR_STAT_TX_EMPTY = (1 << 4), : INTR_STAT_RD_REQ = (1 << 5), : INTR_STAT_TX_ABORT = (1 << 6), : INTR_STAT_RX_DONE = (1 << 7), : INTR_STAT_ACTIVITY = (1 << 8), : INTR_STAT_STOP_DET = (1 << 9), : INTR_STAT_START_DET = (1 << 10), : INTR_STAT_GEN_CALL = (1 << 11), : }; Why do these need to be exported in the header file? I think we can put these in lpss_i2c.c
https://review.coreboot.org/#/c/19468/6/src/soc/intel/common/block/include/i... File src/soc/intel/common/block/include/intelblocks/lpss_i2c.h:
Line 77: /* Initialize I2C Controller */ in early stages before memory is up.
PS6, Line 78: int What are the expected return values?
PS6, Line 80: byt by
PS6, Line 82: GSPI I2C
PS6, Line 102: Driver provides : * weak implementation with default I2C-bus configuration. That is not true.