Ren Kuo has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45329 )
Change subject: mb/google/dedede/var/magolor: Add ACPI camera support ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45329/6/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/magolor/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/45329/6/src/mainboard/google/dedede... PS6, Line 175: register "on_seq.ops[1]" = "SEQ_OPS_GPIO_ENABLE(0, 5)" : register "on_seq.ops[2]" = "SEQ_OPS_GPIO_ENABLE(1, 5)"
Is there a 5ms delay required between individual GPIO enables here? In the static ASL file, there wa […]
Yes, GPIO enable delay = 5ms , run in firmware's boot up duration
coreboot/src/driver/intel/mipi_camera/chip.h
#define SEQ_OPS_GPIO_ENABLE(ind, delay) \ { .type = GPIO, .index = (ind), .action = ENABLE, .delay_ms = (delay) }
it should be executed in the driver of coreboot. coreboot/src/driver/intel/mipi_camera/camera.c
https://review.coreboot.org/c/coreboot/+/45329/6/src/mainboard/google/dedede... PS6, Line 195: ""\_SB.PCI0.I2C3.CAM1.PRIC""
Why is it relying on CAM1 Power resource when there is no CAM1?
ok. it should be CAM0 as definition
https://review.coreboot.org/c/coreboot/+/45329/6/src/mainboard/google/dedede... PS6, Line 200: end
There is no NVM present in this camera module?
File: coreboot/src/driver/intel/mipi_camera/chip.h
/* * Settings specific to nvram. Many values, if left as zero, will be assigned a default. * Set disable_nvm_defaults to non-zero if you want to disable the defaulting behavior * so you can use zero for a value. */ bool disable_nvm_defaults; uint32_t nvm_size; uint32_t nvm_pagesize; uint32_t nvm_readonly; uint32_t nvm_width; const char *nvm_compat;
File: coreboot/src/driver/intel/mipi_camera/camera.c
static void camera_fill_nvm(const struct device *dev) { struct drivers_intel_mipi_camera_config *config = dev->chip_info; struct acpi_dp *dsd = acpi_dp_new_table("_DSD");
if (!config->nvm_compat) return;
/* It might be possible to default size or width based on type. */ if (!config->disable_nvm_defaults && !config->nvm_pagesize) config->nvm_pagesize = 1;
if (!config->disable_nvm_defaults && !config->nvm_readonly) config->nvm_readonly = 1;
if (config->nvm_size) acpi_dp_add_integer(dsd, "size", config->nvm_size);
if (config->nvm_pagesize) acpi_dp_add_integer(dsd, "pagesize", config->nvm_pagesize);
if (config->nvm_readonly) acpi_dp_add_integer(dsd, "read-only", config->nvm_readonly);
if (config->nvm_width) acpi_dp_add_integer(dsd, "address-width", config->nvm_width);
acpi_dp_add_string(dsd, "compatible", config->nvm_compat); acpi_dp_write(dsd); }
About the EERPOM device, I'm not sure we should use "default value" or not ?? The device will be initial in mipi_camear driver.
"Shawnx.tu@intel.com" has provided the setting in previous device tree as following:
chip drivers/intel/mipi_camera register "acpi_hid" = "ACPI_DT_NAMESPACE_HID" register "acpi_uid" = "1" register "acpi_name" = ""NVM0"" register "chip_name" = ""GT24C08"" register "device_type" = "INTEL_ACPI_CAMERA_NVM"
register "pr0" = ""\_SB.PCI0.I2C3.CAM0.PRIC""
register "nvm_size" = "0x2000" register "nvm_pagesize" = "1" register "nvm_readonly" = "1" register "nvm_width" = "0x10" register "nvm_compat" = ""atmel,24c08""
device i2c 50 on end end