[coreboot-gerrit] Change in coreboot[master]: intel/common/block/i2c: Add common block for I2C
Furquan Shaikh (Code Review)
gerrit at coreboot.org
Sat Apr 29 03:20:00 CEST 2017
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_early.c
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_i2c.c
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_i2c.h
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/intelblocks/lpss_i2c.h
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.
--
To view, visit https://review.coreboot.org/19468
To unsubscribe, visit https://review.coreboot.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d91a04c22e181e3a995112cce6d5f0324130b81
Gerrit-PatchSet: 6
Gerrit-Project: coreboot
Gerrit-Branch: master
Gerrit-Owner: Rizwan Qureshi <rizwan.qureshi at intel.com>
Gerrit-Reviewer: Aamir Bohra <aamir.bohra at intel.com>
Gerrit-Reviewer: Aaron Durbin <adurbin at chromium.org>
Gerrit-Reviewer: Bora Guvendik <bora.guvendik at intel.com>
Gerrit-Reviewer: Furquan Shaikh <furquan at google.com>
Gerrit-Reviewer: Hannah Williams <hannah.williams at intel.com>
Gerrit-Reviewer: Lee Leahy <leroy.p.leahy at intel.com>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins)
Gerrit-HasComments: Yes
More information about the coreboot-gerrit
mailing list