Yu-Ping Wu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: vendorcode/google/chromeos: Add camera info ......................................................................
vendorcode/google/chromeos: Add camera info
Add cros_camera_info struct for camera information, and check_cros_camera_info() for checking the magic, CRC and version.
BUG=b:144820097 TEST=emerge-kukui coreboot BRANCH=kukui
Change-Id: I1215fec76643b0cf7e09433e1190e8bd387e6953 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- M src/vendorcode/google/chromeos/Makefile.inc A src/vendorcode/google/chromeos/camera.c A src/vendorcode/google/chromeos/camera.h 3 files changed, 67 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/46042/1
diff --git a/src/vendorcode/google/chromeos/Makefile.inc b/src/vendorcode/google/chromeos/Makefile.inc index e17236d..be6240b 100644 --- a/src/vendorcode/google/chromeos/Makefile.inc +++ b/src/vendorcode/google/chromeos/Makefile.inc @@ -4,6 +4,7 @@ ramstage-$(CONFIG_HAVE_ACPI_TABLES) += gnvs.c ramstage-$(CONFIG_HAVE_ACPI_TABLES) += acpi.c ramstage-$(CONFIG_CHROMEOS_RAMOOPS) += ramoops.c +ramstage-y += camera.c ramstage-y += vpd_mac.c vpd_serialno.c vpd_calibration.c ramstage-$(CONFIG_CHROMEOS_DISABLE_PLATFORM_HIERARCHY_ON_RESUME) += tpm2.c ramstage-$(CONFIG_HAVE_REGULATORY_DOMAIN) += wrdd.c diff --git a/src/vendorcode/google/chromeos/camera.c b/src/vendorcode/google/chromeos/camera.c new file mode 100644 index 0000000..71e28f6 --- /dev/null +++ b/src/vendorcode/google/chromeos/camera.c @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <console/console.h> +#include <crc_byte.h> +#include <string.h> + +#include "camera.h" + +int check_cros_camera_info(const struct cros_camera_info *info) +{ + if (memcmp(info->magic, CROS_CAMERA_INFO_MAGIC, sizeof(info->magic))) { + printk(BIOS_ERR, "Invalid magic in camera info\n"); + return -1; + } + + const uint8_t *ptr = (void *)&info->version; + uint16_t crc16 = 0; + while (ptr < (uint8_t *)info + sizeof(struct cros_camera_info)) + crc16 = crc16_byte(crc16, *ptr++); + + if (info->crc16 != crc16) { + printk(BIOS_ERR, "Incorrect CRC16: expected %#06x, got %#06x\n", + crc16, info->crc16); + return -1; + } + + if (info->version != CROS_CAMERA_INFO_VERSION) { + printk(BIOS_ERR, "Unknown camera info version: %u\n", + info->version); + return -1; + } + if (info->size < CROS_CAMERA_INFO_SIZE_MIN) { + printk(BIOS_ERR, "Size of camera info is too small: %u\n", + info->size); + return -1; + } + + return 0; +} diff --git a/src/vendorcode/google/chromeos/camera.h b/src/vendorcode/google/chromeos/camera.h new file mode 100644 index 0000000..36b7646 --- /dev/null +++ b/src/vendorcode/google/chromeos/camera.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __VENDORCODE_GOOGLE_CHROMEOS_CAMERA_H +#define __VENDORCODE_GOOGLE_CHROMEOS_CAMERA_H + +#include <stdint.h> + +#define CROS_CAMERA_INFO_MAGIC "CrOS" +#define CROS_CAMERA_INFO_VERSION 1 +#define CROS_CAMERA_INFO_SIZE_MIN 0x0a + +struct cros_camera_info { + uint8_t magic[4]; /* CROS_CAMERA_INFO_MAGIC */ + uint16_t crc16; + uint8_t version; + uint8_t size; + uint16_t data_format; + uint16_t module_revision; + uint8_t module_vendor[2]; + uint8_t sensor_vendor[2]; + uint16_t sensor_model; +}; + +/* Returns 0 on success, non-zero on errors. */ +int check_cros_camera_info(const struct cros_camera_info *info); + +#endif
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: vendorcode/google/chromeos: Add camera info ......................................................................
Patch Set 1: Code-Review+2
Tomasz Figa has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: vendorcode/google/chromeos: Add camera info ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46042/1/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/camera.h:
https://review.coreboot.org/c/coreboot/+/46042/1/src/vendorcode/google/chrom... PS1, Line 18: uint16_t module_revision; : uint8_t module_vendor[2]; : uint8_t sensor_vendor[2]; : uint16_t sensor_model; : Could we call these module_pid, module_vid, sensor_vid, sensor_pid respectively, to match the later changes to the design?
https://review.coreboot.org/c/coreboot/+/46042/1/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/camera.c:
https://review.coreboot.org/c/coreboot/+/46042/1/src/vendorcode/google/chrom... PS1, Line 16: &info->version; nit: Would it make sense to make it (void *)(&info->crc16 + 1), so that it always covers all the fields past the crc, even if we make some changes to the struct definition?
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: vendorcode/google/chromeos: Add camera info ......................................................................
Patch Set 1:
Given vendorcode/google/chromeos is only picked when CONFIG_CHROMEOS is defined (see next CL), I'm now wondering should we move this to driver/camera given this is actually a hardware information that will be needed (for identifying SKU) on Chromebooks, not just devices running ChromeOS.
@tfiga, do you think such code should still live in google/chromeos, or move to src/drivers/[camera?] ?
Tomasz Figa has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: vendorcode/google/chromeos: Add camera info ......................................................................
Patch Set 1:
Patch Set 1:
Given vendorcode/google/chromeos is only picked when CONFIG_CHROMEOS is defined (see next CL), I'm now wondering should we move this to driver/camera given this is actually a hardware information that will be needed (for identifying SKU) on Chromebooks, not just devices running ChromeOS.
@tfiga, do you think such code should still live in google/chromeos, or move to src/drivers/[camera?] ?
I guess ideally it would make sense to allow building this without CONFIG_CHROMEOS, although I wonder if in practice that would ever happen? Does anyone ever attempt to build coreboot for a Chromebook without CONFIG_CHROMEOS?
My preference would be something like drivers/camera/chromeos-eeprom.c, but I don't have much experience with coreboot, so please do whatever appropriate. :)
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: vendorcode/google/chromeos: Add camera info ......................................................................
Patch Set 1:
I guess ideally it would make sense to allow building this without CONFIG_CHROMEOS, although I wonder if in practice that would ever happen? Does anyone ever attempt to build coreboot for a Chromebook without CONFIG_CHROMEOS?
Not just Coreboot, people who try to build uboot may still want to boot Chrome OS kernel using same config (device tree).
My preference would be something like drivers/camera/chromeos-eeprom.c, but I don't have much experience with coreboot, so please do whatever appropriate. :)
Then I'd vote for drivers/camera/chrome_eeprom.c
Maybe not calling it "chromeos" given we're actually dealing with "camera on chromebooks" (e.g., hw instead of sw).
Tomasz Figa has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: vendorcode/google/chromeos: Add camera info ......................................................................
Patch Set 1:
Patch Set 1:
I guess ideally it would make sense to allow building this without CONFIG_CHROMEOS, although I wonder if in practice that would ever happen? Does anyone ever attempt to build coreboot for a Chromebook without CONFIG_CHROMEOS?
Not just Coreboot, people who try to build uboot may still want to boot Chrome OS kernel using same config (device tree).
My preference would be something like drivers/camera/chromeos-eeprom.c, but I don't have much experience with coreboot, so please do whatever appropriate. :)
Then I'd vote for drivers/camera/chrome_eeprom.c
Maybe not calling it "chromeos" given we're actually dealing with "camera on chromebooks" (e.g., hw instead of sw).
Hmm, it might be just me, but chrome kind of suggests the browser. In the kernel we already have some drivers like for the EC, which is called cros_ec. Perhaps we could follow the suit and use cros_eeprom? IMHO it still makes sense, because even though one could use it on another system, it was designed by and for Chrome OS.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: vendorcode/google/chromeos: Add camera info ......................................................................
Patch Set 1:
Hmm, it might be just me, but chrome kind of suggests the browser. In the kernel we already have some drivers like for the EC, which is called cros_ec. Perhaps we could follow the suit and use cros_eeprom? IMHO it still makes sense, because even though one could use it on another system, it was designed by and for Chrome OS.
I'm OK with something starting as cros, but I'm not that convinced on 'eeprom'. The real implementation is "cros_camera_info", a structure describing camera attributes. On MIPI it should usually live on EEPROM, but maybe we'll want to read it from some where? Especially there's no real EEPROM driver included (although maybe we should do that in future).
What about just cros_camera or cros_camera_info ?
Tomasz Figa has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: vendorcode/google/chromeos: Add camera info ......................................................................
Patch Set 1:
Patch Set 1:
Hmm, it might be just me, but chrome kind of suggests the browser. In the kernel we already have some drivers like for the EC, which is called cros_ec. Perhaps we could follow the suit and use cros_eeprom? IMHO it still makes sense, because even though one could use it on another system, it was designed by and for Chrome OS.
I'm OK with something starting as cros, but I'm not that convinced on 'eeprom'. The real implementation is "cros_camera_info", a structure describing camera attributes. On MIPI it should usually live on EEPROM, but maybe we'll want to read it from some where? Especially there's no real EEPROM driver included (although maybe we should do that in future).
What about just cros_camera or cros_camera_info ?
I'm okay with either, with a slight preference for cros_camera for being shorter. :)
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, Tomasz Figa,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46042
to look at the new patch set (#2).
Change subject: drivers/camera: Add cros camera info ......................................................................
drivers/camera: Add cros camera info
Add cros_camera_info struct for camera information, and check_cros_camera_info() for checking the magic, CRC and version.
BUG=b:144820097 TEST=emerge-kukui coreboot BRANCH=kukui
Change-Id: I1215fec76643b0cf7e09433e1190e8bd387e6953 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- A src/drivers/camera/Makefile.inc A src/drivers/camera/cros_camera.c A src/drivers/camera/cros_camera.h 3 files changed, 67 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/46042/2
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: drivers/camera: Add cros camera info ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/46042/1/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/camera.h:
https://review.coreboot.org/c/coreboot/+/46042/1/src/vendorcode/google/chrom... PS1, Line 18: uint16_t module_revision; : uint8_t module_vendor[2]; : uint8_t sensor_vendor[2]; : uint16_t sensor_model; :
Could we call these module_pid, module_vid, sensor_vid, sensor_pid respectively, to match the later […]
Should we use uint16_t for *_vid?
https://review.coreboot.org/c/coreboot/+/46042/1/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/camera.c:
https://review.coreboot.org/c/coreboot/+/46042/1/src/vendorcode/google/chrom... PS1, Line 16: &info->version;
nit: Would it make sense to make it (void *)(&info->crc16 + 1), so that it always covers all the fie […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: drivers/camera: Add cros camera info ......................................................................
Patch Set 2:
Thanks for the change, but I think most src/drivers/* folders do have a Kconfig so the driver is selected when some config is set.
Can you add a Kconfig with new config, for example CONFIG_CHROMEOS_CAMERA ?
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, Tomasz Figa,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46042
to look at the new patch set (#3).
Change subject: drivers/camera: Add config CHROMEOS_CAMERA ......................................................................
drivers/camera: Add config CHROMEOS_CAMERA
Add cros_camera_info struct for camera information, and check_cros_camera_info() for checking the magic, CRC and version.
BUG=b:144820097 TEST=emerge-kukui coreboot BRANCH=kukui
Change-Id: I1215fec76643b0cf7e09433e1190e8bd387e6953 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- A src/drivers/camera/Kconfig A src/drivers/camera/Makefile.inc A src/drivers/camera/cros_camera.c A src/drivers/camera/cros_camera.h 4 files changed, 70 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/46042/3
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: drivers/camera: Add config CHROMEOS_CAMERA ......................................................................
Patch Set 3:
Patch Set 2:
Thanks for the change, but I think most src/drivers/* folders do have a Kconfig so the driver is selected when some config is set.
Can you add a Kconfig with new config, for example CONFIG_CHROMEOS_CAMERA ?
Done.
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: drivers/camera: Add config CHROMEOS_CAMERA ......................................................................
Patch Set 3: Code-Review+2
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: drivers/camera: Add config CHROMEOS_CAMERA ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46042/3/src/drivers/camera/Kconfig File src/drivers/camera/Kconfig:
https://review.coreboot.org/c/coreboot/+/46042/3/src/drivers/camera/Kconfig@... PS3, Line 4: Can you add
help Camera with identifiers following Chrome OS Camera Info. The info is usually available on MIPI camera EEPROM for identifying correct drivers and config.
Hello Hung-Te Lin, build bot (Jenkins), Patrick Georgi, Martin Roth, Julius Werner, Tomasz Figa,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46042
to look at the new patch set (#4).
Change subject: drivers/camera: Add config CHROMEOS_CAMERA ......................................................................
drivers/camera: Add config CHROMEOS_CAMERA
Add cros_camera_info struct for camera information, and check_cros_camera_info() for checking the magic, CRC and version.
BUG=b:144820097 TEST=emerge-kukui coreboot BRANCH=kukui
Change-Id: I1215fec76643b0cf7e09433e1190e8bd387e6953 Signed-off-by: Yu-Ping Wu yupingso@chromium.org --- A src/drivers/camera/Kconfig A src/drivers/camera/Makefile.inc A src/drivers/camera/cros_camera.c A src/drivers/camera/cros_camera.h 4 files changed, 74 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/42/46042/4
Yu-Ping Wu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: drivers/camera: Add config CHROMEOS_CAMERA ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46042/3/src/drivers/camera/Kconfig File src/drivers/camera/Kconfig:
https://review.coreboot.org/c/coreboot/+/46042/3/src/drivers/camera/Kconfig@... PS3, Line 4:
Can you add […]
Done
Hung-Te Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: drivers/camera: Add config CHROMEOS_CAMERA ......................................................................
Patch Set 4: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46042 )
Change subject: drivers/camera: Add config CHROMEOS_CAMERA ......................................................................
drivers/camera: Add config CHROMEOS_CAMERA
Add cros_camera_info struct for camera information, and check_cros_camera_info() for checking the magic, CRC and version.
BUG=b:144820097 TEST=emerge-kukui coreboot BRANCH=kukui
Change-Id: I1215fec76643b0cf7e09433e1190e8bd387e6953 Signed-off-by: Yu-Ping Wu yupingso@chromium.org Reviewed-on: https://review.coreboot.org/c/coreboot/+/46042 Reviewed-by: Hung-Te Lin hungte@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- A src/drivers/camera/Kconfig A src/drivers/camera/Makefile.inc A src/drivers/camera/cros_camera.c A src/drivers/camera/cros_camera.h 4 files changed, 74 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Hung-Te Lin: Looks good to me, approved
diff --git a/src/drivers/camera/Kconfig b/src/drivers/camera/Kconfig new file mode 100644 index 0000000..1c0c0a3 --- /dev/null +++ b/src/drivers/camera/Kconfig @@ -0,0 +1,7 @@ +config CHROMEOS_CAMERA + bool + default n + help + Camera with identifiers following Chrome OS Camera Info. The info is + usually available on MIPI camera EEPROM for identifying correct + drivers and config. diff --git a/src/drivers/camera/Makefile.inc b/src/drivers/camera/Makefile.inc new file mode 100644 index 0000000..1a6e609 --- /dev/null +++ b/src/drivers/camera/Makefile.inc @@ -0,0 +1 @@ +ramstage-$(CONFIG_CHROMEOS_CAMERA) += cros_camera.c diff --git a/src/drivers/camera/cros_camera.c b/src/drivers/camera/cros_camera.c new file mode 100644 index 0000000..ff39678 --- /dev/null +++ b/src/drivers/camera/cros_camera.c @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#include <console/console.h> +#include <crc_byte.h> +#include <string.h> + +#include "cros_camera.h" + +int check_cros_camera_info(const struct cros_camera_info *info) +{ + if (memcmp(info->magic, CROS_CAMERA_INFO_MAGIC, sizeof(info->magic))) { + printk(BIOS_ERR, "Invalid magic in camera info\n"); + return -1; + } + + const uint8_t *ptr = (void *)(&info->crc16 + 1); + uint16_t crc16 = 0; + while (ptr < (uint8_t *)info + sizeof(struct cros_camera_info)) + crc16 = crc16_byte(crc16, *ptr++); + + if (info->crc16 != crc16) { + printk(BIOS_ERR, "Incorrect CRC16: expected %#06x, got %#06x\n", + crc16, info->crc16); + return -1; + } + + if (info->version != CROS_CAMERA_INFO_VERSION) { + printk(BIOS_ERR, "Unknown camera info version: %u\n", + info->version); + return -1; + } + if (info->size < CROS_CAMERA_INFO_SIZE_MIN) { + printk(BIOS_ERR, "Size of camera info is too small: %u\n", + info->size); + return -1; + } + + return 0; +} diff --git a/src/drivers/camera/cros_camera.h b/src/drivers/camera/cros_camera.h new file mode 100644 index 0000000..f69e77d --- /dev/null +++ b/src/drivers/camera/cros_camera.h @@ -0,0 +1,27 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ + +#ifndef __VENDORCODE_GOOGLE_CHROMEOS_CAMERA_H +#define __VENDORCODE_GOOGLE_CHROMEOS_CAMERA_H + +#include <stdint.h> + +#define CROS_CAMERA_INFO_MAGIC "CrOS" +#define CROS_CAMERA_INFO_VERSION 1 +#define CROS_CAMERA_INFO_SIZE_MIN 0x0a + +struct cros_camera_info { + uint8_t magic[4]; /* CROS_CAMERA_INFO_MAGIC */ + uint16_t crc16; + uint8_t version; + uint8_t size; + uint16_t data_format; + uint16_t module_pid; + uint8_t module_vid[2]; + uint8_t sensor_vid[2]; + uint16_t sensor_pid; +}; + +/* Returns 0 on success, non-zero on errors. */ +int check_cros_camera_info(const struct cros_camera_info *info); + +#endif
-
Hung-Te Lin (Code Review) -
Patrick Georgi (Code Review)
-
Tomasz Figa (Code Review)
-
Yu-Ping Wu (Code Review)