Rizwan Qureshi 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:
(25 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)
yep, looks clean.
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
Ideally yes, If not initialized in bootblock/romstage, FSP should bring these controller out of reset for sure. In ramstage if I want to bring the controller out of reset, we will need a temporary base we cant use the EARLY_BASE since the memory has been initialized by now.
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 22: #include <device/pci_ids.h>
not needed?
Done
PS6, Line 29: DEVTREE_CONST
just make this const
Done
PS6, Line 30: DEVTREE_CONST
just make it const
Done
Line 51: config = i2c_get_soc_cfg();
This is sorta odd in that we check for the device above. But the implemente
Done
PS6, Line 52: {
space before {
Done
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.
not an error, will keep it as BIOS_DEBUG
PS6, Line 84: 0
(uintptr_t)NULL
Done
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: }
It was moved to intel/common/block/i2c/i2c.c
want to keep the acpigen related things in i2c.c
Line 729: }
Moved to intel/common/block/i2c/i2c.c
right
PS6, Line 447: int
I think because this is called from lpss_i2c_acpi_fill_ssdt which is now mo
right
Line 592: int lpss_i2c_bus_to_dev(int bus)
Why is this global?
to be accessible in i2c.c/i2c_early.c, got rid of them anyway.
Line 597: int lpss_i2c_dev_to_bus(struct device *dev)
likewise
Done
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->
Done
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 compilati
I thought of the same thing at first then felt that a compilation failure is too extreme for this. will restore the failure.
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
Will restore it.
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 the
moved
Line 149: int lpss_i2c_bus_to_dev(int bus);
Why did this lose the devfn distinction?
removed these
Line 154: int lpss_i2c_gen_speed_config(struct lpss_i2c_regs *regs,
Why is this exposed?
will be used in blocks/i2c/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.
made this static in i2c_early.c
PS6, Line 78: int
What are the expected return values?
Done
PS6, Line 80: byt
by
Done
PS6, Line 82: GSPI
I2C
Done
PS6, Line 102: Driver provides : * weak implementation with default I2C-bus configuration.
That is not true.
Done