[coreboot-gerrit] Change in coreboot[master]: intel/common/block/i2c: Add common block for I2C
Rizwan Qureshi (Code Review)
gerrit at coreboot.org
Wed May 3 16:56:12 CEST 2017
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_early.c
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_i2c.c
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_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
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/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.
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
--
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: Paul Menzel <paulepanter at users.sourceforge.net>
Gerrit-Reviewer: Rizwan Qureshi <rizwan.qureshi at intel.com>
Gerrit-Reviewer: Subrata Banik <subrata.banik at intel.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply at coreboot.org>
Gerrit-HasComments: Yes
More information about the coreboot-gerrit
mailing list