Hello Hung-Te Lin,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/45811
to review the following change.
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
mb/google/kukui: Support SKU from camera EEPROM
To support camera second source GC5035 for kodama, add world facing camera id as part of the sku id, which is determined by the data in camera EEPROM. For models other than kodama, the camera id is always 0 and hence the sku id is unchanged.
BUG=b:144820097 TEST=emerge-kukui coreboot TEST=Correct WFC id detected for kodama with GC5035 camera BRANCH=kukui
Change-Id: I63a2b952b8c35c0ead8200d7c926e8d90a9f3fb8 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/boardid.c 2 files changed, 102 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/45811/1
diff --git a/src/mainboard/google/kukui/Makefile.inc b/src/mainboard/google/kukui/Makefile.inc index 532712a..7e065a3 100644 --- a/src/mainboard/google/kukui/Makefile.inc +++ b/src/mainboard/google/kukui/Makefile.inc @@ -1,7 +1,6 @@ subdirs-y += sdram_params/ subdirs-y += panel_params/
-bootblock-y += boardid.c bootblock-y += bootblock.c bootblock-y += reset.c
diff --git a/src/mainboard/google/kukui/boardid.c b/src/mainboard/google/kukui/boardid.c index 548f36b..f8219fb 100644 --- a/src/mainboard/google/kukui/boardid.c +++ b/src/mainboard/google/kukui/boardid.c @@ -3,8 +3,14 @@ #include <assert.h> #include <boardid.h> #include <console/console.h> -#include <soc/auxadc.h> +#include <delay.h> +#include <device/i2c_simple.h> #include <ec/google/chromeec/ec.h> +#include <soc/auxadc.h> +#include <soc/i2c.h> +#include <soc/pmic_wrap_common.h> +#include <string.h> +#include <vendorcode/google/chromeos/camera.h>
/* For CBI un-provisioned/corrupted Flapjack board. */ #define FLAPJACK_UNDEF_SKU_ID 0 @@ -72,6 +78,97 @@ return id; }
+static uint8_t eeprom_random_read(uint8_t bus, uint8_t slave, uint16_t offset, + uint8_t *data, uint16_t len) +{ + struct i2c_msg seg[2]; + uint8_t address[2]; + + address[0] = offset >> 8; + address[1] = offset & 0xff; + + seg[0].flags = 0; + seg[0].slave = slave; + seg[0].buf = address; + seg[0].len = sizeof(address); + seg[1].flags = I2C_M_RD; + seg[1].slave = slave; + seg[1].buf = data; + seg[1].len = len; + + return i2c_transfer(bus, seg, ARRAY_SIZE(seg)); +} + +/* Regulator for world facing camera. */ +#define PMIC_LDO_VCAMIO_CON0 0x1cb0 + +#define CROS_CAMERA_INFO_OFFSET 0x1f80 +#define MT8183_FORMAT 0x8183 +#define KODAMA_REVISION 0x00c7 + +/* Returns the ID for world facing camera. */ +static uint8_t wfc_id(void) +{ + if (!CONFIG(BOARD_GOOGLE_KODAMA)) + return 0; + + int i, ret; + uint8_t bus = 2; + uint8_t dev_addr = 0x50; /* at24c32/64 device address */ + + struct cros_camera_info data = {0}; + + const uint16_t sensor_models[] = { + [0] = 0x5965, /* OV5965 */ + [1] = 0x5035, /* GC5035 */ + }; + + mtk_i2c_bus_init(bus); + + /* Turn on camera sensor EEPROM */ + pwrap_write(PMIC_LDO_VCAMIO_CON0, 0x1); + udelay(270); + + ret = eeprom_random_read(bus, dev_addr, CROS_CAMERA_INFO_OFFSET, + (uint8_t *)&data, sizeof(data)); + pwrap_write(PMIC_LDO_VCAMIO_CON0, 0x0); + + if (ret) { + printk(BIOS_ERR, + "Failed to read from EEPROM; using default WFC id 0\n"); + return 0; + } + + if (check_cros_camera_info(&data)) { + printk(BIOS_ERR, + "Failed to check camera info; using default WFC id 0\n"); + return 0; + } + + if (data.data_format != MT8183_FORMAT) { + printk(BIOS_ERR, "Incompatible camera format: %#04x\n", + data.data_format); + return 0; + } + if (data.module_revision != KODAMA_REVISION) { + printk(BIOS_ERR, "Incompatible module revision: %#04x\n", + data.module_revision); + return 0; + } + + printk(BIOS_DEBUG, "Camera sensor model: %#04x\n", data.sensor_model); + + for (i = 0; i < ARRAY_SIZE(sensor_models); i++) { + if (data.sensor_model == sensor_models[i]) { + printk(BIOS_INFO, "Detected WFC id: %d\n", i); + return i; + } + } + + printk(BIOS_WARNING, "Unknown WFC id; using default id 0\n"); + return 0; +} + /* board_id is provided by ec/google/chromeec/ec_boardid.c */
uint32_t sku_id(void) @@ -98,11 +195,14 @@
/* * The SKU (later used for device tree matching) is combined from: + * World facing camera (WFC) ID. * ADC2[4bit/H] = straps on LCD module (type of panel). * ADC4[4bit/L] = SKU ID from board straps. */ - cached_sku_id = (get_adc_index(LCM_ID_CHANNEL) << 4 | + cached_sku_id = (wfc_id() << 8 | + get_adc_index(LCM_ID_CHANNEL) << 4 | get_adc_index(SKU_ID_CHANNEL)); + return cached_sku_id; }
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 1:
(1 comment)
LGTM if you can fix the build failure.
https://review.coreboot.org/c/coreboot/+/45811/1/src/mainboard/google/kukui/... File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/c/coreboot/+/45811/1/src/mainboard/google/kukui/... PS1, Line 13: vendorcode/google/chromeos/camera.h did you forget to upload the patch for this file?
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45811/1/src/mainboard/google/kukui/... File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/c/coreboot/+/45811/1/src/mainboard/google/kukui/... PS1, Line 13: vendorcode/google/chromeos/camera.h
did you forget to upload the patch for this file?
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 2:
I wonder how we can trigger rebuild
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 2:
undefined reference to `check_cros_camera_info'
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45811/2/src/mainboard/google/kukui/... File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/c/coreboot/+/45811/2/src/mainboard/google/kukui/... PS2, Line 142: check_cros_camera_info this is defined in chromeos/Makefile.inc, which will be processed only by rule subdirs-$(CONFIG_CHROMEOS) += chromeos
So we can only invoke this function when CONFIG_CHROMEOS is set.
Can you try if add a
if (!CONFIG(CHROMEOS)) return 0;
In the beginning of this file and see if compiler optimization would skip linking check_cros_camera_info?
Otherwise I think we'll need to use #ifdef.
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45811/2/src/mainboard/google/kukui/... File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/c/coreboot/+/45811/2/src/mainboard/google/kukui/... PS2, Line 142: check_cros_camera_info
this is defined in chromeos/Makefile.inc, which will be processed only by rule […]
When CONFIG(BOARD_GOOGLE_KODAMA), shouldn't CONFIG(CHROMEOS) also be 1? I can run
util/abuild/abuild -c max -t GOOGLE_KODAMA -x
locally without errors. The '-x' option of abuild will set CONFIG_CHROMEOS=y, so I wonder why we got this error. From the log, I can't tell if the error comes from [1] or other lines.
[1] https://github.com/coreboot/coreboot/blob/6e8d38ec4e6a5a89e1bf6cec611d5bf8d4...
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45811/2/src/mainboard/google/kukui/... File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/c/coreboot/+/45811/2/src/mainboard/google/kukui/... PS2, Line 142: check_cros_camera_info
When CONFIG(BOARD_GOOGLE_KODAMA), shouldn't CONFIG(CHROMEOS) also be 1? I can run
No. The kukui/Kconfig only sets MAINBOARD_HAS_CHROMEOS. And CHROMEOS is default n (that it'll be set to y if we're building with emerge)
util/abuild/abuild -c max -t GOOGLE_KODAMA -x
But I think the CQ failed when building without CHROMEOS
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45811
to look at the new patch set (#3).
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
mb/google/kukui: Support SKU from camera EEPROM
To support camera second source GC5035 for kodama, add world facing camera id as part of the sku id, which is determined by the data in camera EEPROM. For models other than kodama, the camera id is always 0 and hence the sku id is unchanged.
BUG=b:144820097 TEST=emerge-kukui coreboot TEST=Correct WFC id detected for kodama with GC5035 camera BRANCH=kukui
Change-Id: I63a2b952b8c35c0ead8200d7c926e8d90a9f3fb8 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/boardid.c 2 files changed, 104 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/45811/3
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45811/2/src/mainboard/google/kukui/... File src/mainboard/google/kukui/boardid.c:
https://review.coreboot.org/c/coreboot/+/45811/2/src/mainboard/google/kukui/... PS2, Line 142: check_cros_camera_info
When CONFIG(BOARD_GOOGLE_KODAMA), shouldn't CONFIG(CHROMEOS) also be 1? I can run […]
Ack
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45811
to look at the new patch set (#4).
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
mb/google/kukui: Support SKU from camera EEPROM
To support camera second source GC5035 for kodama, add world facing camera id as part of the sku id, which is determined by the data in camera EEPROM. For models other than kodama, the camera id is always 0 and hence the sku id is unchanged.
BUG=b:144820097 TEST=emerge-kukui coreboot TEST=Correct WFC id detected for kodama with GC5035 camera BRANCH=kukui
Change-Id: I63a2b952b8c35c0ead8200d7c926e8d90a9f3fb8 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/boardid.c 2 files changed, 102 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/45811/4
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45811
to look at the new patch set (#5).
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
mb/google/kukui: Support SKU from camera EEPROM
To support camera second source GC5035 for kodama, add world facing camera id as part of the sku id, which is determined by the data in camera EEPROM. For models other than kodama, the camera id is always 0 and hence the sku id is unchanged.
BUG=b:144820097 TEST=emerge-kukui coreboot TEST=Correct WFC id detected for kodama with GC5035 camera BRANCH=kukui
Change-Id: I63a2b952b8c35c0ead8200d7c926e8d90a9f3fb8 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/mainboard/google/kukui/Kconfig.name M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/boardid.c 3 files changed, 105 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/45811/5
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45811/5/src/mainboard/google/kukui/... File src/mainboard/google/kukui/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/45811/5/src/mainboard/google/kukui/... PS5, Line 14: select CHROMEOS_CAMERA let's keep Kconfig.name simple. Instead you should put add a
select CHROMEOS_CAMERA if BOARD_GOOGLE_KODAMA
in Kconfig BOARD_SPECIFIC_OPTIONS section.
In fact, since we're in public drivers now, I think it's totally fine to always include the driver:
select CHROMEOS_CAMERA
then you don't have to 'return 0' by checking CONFIG_CHROMEOS_CAMERA.
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45811
to look at the new patch set (#6).
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
mb/google/kukui: Support SKU from camera EEPROM
To support camera second source GC5035 for kodama, add world facing camera id as part of the sku id, which is determined by the data in camera EEPROM. For models other than kodama, the camera id is always 0 and hence the sku id is unchanged.
BUG=b:144820097 TEST=emerge-kukui coreboot TEST=Correct WFC id detected for kodama with GC5035 camera BRANCH=kukui
Change-Id: I63a2b952b8c35c0ead8200d7c926e8d90a9f3fb8 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/mainboard/google/kukui/Kconfig M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/boardid.c 3 files changed, 103 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/11/45811/6
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45811/5/src/mainboard/google/kukui/... File src/mainboard/google/kukui/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/45811/5/src/mainboard/google/kukui/... PS5, Line 14: select CHROMEOS_CAMERA
let's keep Kconfig.name simple. Instead you should put add a […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
Patch Set 6: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45811 )
Change subject: mb/google/kukui: Support SKU from camera EEPROM ......................................................................
mb/google/kukui: Support SKU from camera EEPROM
To support camera second source GC5035 for kodama, add world facing camera id as part of the sku id, which is determined by the data in camera EEPROM. For models other than kodama, the camera id is always 0 and hence the sku id is unchanged.
BUG=b:144820097 TEST=emerge-kukui coreboot TEST=Correct WFC id detected for kodama with GC5035 camera BRANCH=kukui
Change-Id: I63a2b952b8c35c0ead8200d7c926e8d90a9f3fb8 Signed-off-by: Yu-Ping Wu yupingso@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/45811 Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/mainboard/google/kukui/Kconfig M src/mainboard/google/kukui/Makefile.inc M src/mainboard/google/kukui/boardid.c 3 files changed, 103 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/mainboard/google/kukui/Kconfig b/src/mainboard/google/kukui/Kconfig index 4e308ef..2237efa 100644 --- a/src/mainboard/google/kukui/Kconfig +++ b/src/mainboard/google/kukui/Kconfig @@ -23,6 +23,7 @@ select SOC_MEDIATEK_MT8183 select BOARD_ROMSIZE_KB_8192 select MAINBOARD_HAS_CHROMEOS + select CHROMEOS_CAMERA select CHROMEOS_USE_EC_WATCHDOG_FLAG if CHROMEOS select COMMON_CBFS_SPI_WRAPPER select SPI_FLASH diff --git a/src/mainboard/google/kukui/Makefile.inc b/src/mainboard/google/kukui/Makefile.inc index 532712a..7e065a3 100644 --- a/src/mainboard/google/kukui/Makefile.inc +++ b/src/mainboard/google/kukui/Makefile.inc @@ -1,7 +1,6 @@ subdirs-y += sdram_params/ subdirs-y += panel_params/
-bootblock-y += boardid.c bootblock-y += bootblock.c bootblock-y += reset.c
diff --git a/src/mainboard/google/kukui/boardid.c b/src/mainboard/google/kukui/boardid.c index 548f36b..47b0d9b 100644 --- a/src/mainboard/google/kukui/boardid.c +++ b/src/mainboard/google/kukui/boardid.c @@ -3,8 +3,14 @@ #include <assert.h> #include <boardid.h> #include <console/console.h> -#include <soc/auxadc.h> +#include <delay.h> +#include <device/i2c_simple.h> +#include <drivers/camera/cros_camera.h> #include <ec/google/chromeec/ec.h> +#include <soc/auxadc.h> +#include <soc/i2c.h> +#include <soc/pmic_wrap_common.h> +#include <string.h>
/* For CBI un-provisioned/corrupted Flapjack board. */ #define FLAPJACK_UNDEF_SKU_ID 0 @@ -72,6 +78,97 @@ return id; }
+static uint8_t eeprom_random_read(uint8_t bus, uint8_t slave, uint16_t offset, + uint8_t *data, uint16_t len) +{ + struct i2c_msg seg[2]; + uint8_t address[2]; + + address[0] = offset >> 8; + address[1] = offset & 0xff; + + seg[0].flags = 0; + seg[0].slave = slave; + seg[0].buf = address; + seg[0].len = sizeof(address); + seg[1].flags = I2C_M_RD; + seg[1].slave = slave; + seg[1].buf = data; + seg[1].len = len; + + return i2c_transfer(bus, seg, ARRAY_SIZE(seg)); +} + +/* Regulator for world facing camera. */ +#define PMIC_LDO_VCAMIO_CON0 0x1cb0 + +#define CROS_CAMERA_INFO_OFFSET 0x1f80 +#define MT8183_FORMAT 0x8183 +#define KODAMA_PID 0x00c7 + +/* Returns the ID for world facing camera. */ +static uint8_t wfc_id(void) +{ + if (!CONFIG(BOARD_GOOGLE_KODAMA)) + return 0; + + int i, ret; + uint8_t bus = 2; + uint8_t dev_addr = 0x50; /* at24c32/64 device address */ + + struct cros_camera_info data = {0}; + + const uint16_t sensor_pids[] = { + [0] = 0x5965, /* OV5965 */ + [1] = 0x5035, /* GC5035 */ + }; + + mtk_i2c_bus_init(bus); + + /* Turn on camera sensor EEPROM */ + pwrap_write(PMIC_LDO_VCAMIO_CON0, 0x1); + udelay(270); + + ret = eeprom_random_read(bus, dev_addr, CROS_CAMERA_INFO_OFFSET, + (uint8_t *)&data, sizeof(data)); + pwrap_write(PMIC_LDO_VCAMIO_CON0, 0x0); + + if (ret) { + printk(BIOS_ERR, + "Failed to read from EEPROM; using default WFC id 0\n"); + return 0; + } + + if (check_cros_camera_info(&data)) { + printk(BIOS_ERR, + "Failed to check camera info; using default WFC id 0\n"); + return 0; + } + + if (data.data_format != MT8183_FORMAT) { + printk(BIOS_ERR, "Incompatible camera format: %#04x\n", + data.data_format); + return 0; + } + if (data.module_pid != KODAMA_PID) { + printk(BIOS_ERR, "Incompatible module pid: %#04x\n", + data.module_pid); + return 0; + } + + printk(BIOS_DEBUG, "Camera sensor pid: %#04x\n", data.sensor_pid); + + for (i = 0; i < ARRAY_SIZE(sensor_pids); i++) { + if (data.sensor_pid == sensor_pids[i]) { + printk(BIOS_INFO, "Detected WFC id: %d\n", i); + return i; + } + } + + printk(BIOS_WARNING, "Unknown WFC id; using default id 0\n"); + return 0; +} + /* board_id is provided by ec/google/chromeec/ec_boardid.c */
uint32_t sku_id(void) @@ -98,11 +195,14 @@
/* * The SKU (later used for device tree matching) is combined from: + * World facing camera (WFC) ID. * ADC2[4bit/H] = straps on LCD module (type of panel). * ADC4[4bit/L] = SKU ID from board straps. */ - cached_sku_id = (get_adc_index(LCM_ID_CHANNEL) << 4 | + cached_sku_id = (wfc_id() << 8 | + get_adc_index(LCM_ID_CHANNEL) << 4 | get_adc_index(SKU_ID_CHANNEL)); + return cached_sku_id; }
-
Hung-Te Lin (Code Review) -
Patrick Georgi (Code Review)
-
Yu-Ping Wu (Code Review)