Change in coreboot[master]: drivers/intel/mipi_camera: SSDT changes to handle CIO2 device
Hello Matt Delco,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/42470
to review the following change.
Change subject: drivers/intel/mipi_camera: SSDT changes to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: SSDT changes to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 113 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42470/1
diff --git a/src/drivers/intel/mipi_camera/camera.c b/src/drivers/intel/mipi_camera/camera.c index ddafccf..bcce7a6 100644 --- a/src/drivers/intel/mipi_camera/camera.c +++ b/src/drivers/intel/mipi_camera/camera.c @@ -10,9 +10,83 @@ #include <device/pci_def.h> #include "chip.h"
+#define CIO2_PCI_DEV 0x14 +#define CIO2_PCI_FN 0x3 #define SENSOR_NAME_UUID "822ace8f-2814-4174-a56b-5f029fe079ee" #define SENSOR_TYPE_UUID "26257549-9271-4ca4-bb43-c4899d5a4881"
+/* + * This implementation assumes there is only 1 endpoint at each end of every data port. It also + * assumes there are no clock lanes. It also assumes that any VCM or NVM for a CAM is on the + * same I2C bus as the CAM. + */ + +/* + * Adds settings for a CIO2 device (typically at "_SB.PCI0.CIO2"). A _DSD is added that + * specifies a child table for each port (e.g., PRT0 for "port0" and PRT1 for "port1"). Each + * PRTx table specifies a table for each endpoint (though only 1 endpoint is supported by this + * implementation so the table only has an "endpoint0" that points to a EPx0 table). The EPx0 + * table primarily describes the # of lanes in "data-lines" and specifies the name of the other + * side in "remote-endpoint" (the name is usually of the form "_SB.PCI0.I2Cx.CAM0" for the + * first port/cam and "_SB.PCI0.I2Cx.CAM1" if there's a second port/cam). + */ +static void camera_fill_cio2(const struct device *dev) +{ + struct drivers_intel_mipi_camera_config *config = dev->chip_info; + struct acpi_dp *dsd = acpi_dp_new_table("_DSD"); + char *ep_table_name[MAX_PORT_ENTRIES] = {NULL}; + char *port_table_name[MAX_PORT_ENTRIES] = {NULL}; + char *port_name[MAX_PORT_ENTRIES] = {NULL}; + unsigned int i, j; + char name[BUS_PATH_MAX]; + struct acpi_dp *ep_table = NULL; + struct acpi_dp *port_table = NULL; + struct acpi_dp *remote = NULL; + struct acpi_dp *lanes = NULL; + + for (i = 0; i < config->cio2_num_ports && i < MAX_PORT_ENTRIES; ++i) { + snprintf(name, sizeof(name), "EP%u0", i); + ep_table_name[i] = strdup(name); + ep_table = acpi_dp_new_table(ep_table_name[i]); + acpi_dp_add_integer(ep_table, "endpoint", 0); + acpi_dp_add_integer(ep_table, "clock-lanes", 0); + + if (config->cio2_lanes_used[i] > 0) { + lanes = acpi_dp_new_table("data-lanes"); + + for (j = 1; j <= config->cio2_lanes_used[i]; ++j) + acpi_dp_add_integer(lanes, NULL, j); + acpi_dp_add_array(ep_table, lanes); + } + + if (config->cio2_lane_endpoint[i]) { + remote = acpi_dp_new_table("remote-endpoint"); + acpi_dp_add_reference(remote, NULL, config->cio2_lane_endpoint[i]); + acpi_dp_add_integer(remote, NULL, 0); + acpi_dp_add_integer(remote, NULL, 0); + acpi_dp_add_array(ep_table, remote); + } + + snprintf(name, sizeof(name), "PRT%u", i); + port_table_name[i] = strdup(name); + port_table = acpi_dp_new_table(port_table_name[i]); + acpi_dp_add_integer(port_table, "port", config->cio2_prt[i]); + acpi_dp_add_child(port_table, "endpoint0", ep_table); + + snprintf(name, sizeof(name), "port%u", i); + port_name[i] = strdup(name); + acpi_dp_add_child(dsd, port_name[i], port_table); + } + + acpi_dp_write(dsd); + + for (i = 0; i < config->cio2_num_ports; ++i) { + free(ep_table_name[i]); + free(port_table_name[i]); + free(port_name[i]); + } +} + static void apply_pld_defaults(struct drivers_intel_mipi_camera_config *config) { if (!config->pld.ignore_color) @@ -142,6 +216,11 @@
static void camera_fill_sensor_defaults(struct drivers_intel_mipi_camera_config *config) { + struct device *cio2 = pcidev_on_root(CIO2_PCI_DEV, CIO2_PCI_FN); + struct drivers_intel_mipi_camera_config *cio2_config; + if (!config->ssdb.bdf_value) + config->ssdb.bdf_value = PCI_DEVFN(CIO2_PCI_DEV, CIO2_PCI_FN); + if (!config->ssdb.platform) config->ssdb.platform = PLATFORM_SKC;
@@ -156,6 +235,31 @@
if (!config->ssdb.mclk_speed) config->ssdb.mclk_speed = CLK_FREQ_19_2MHZ; + + if (!config->ssdb.lanes_used) { + cio2_config = cio2 ? cio2->chip_info : NULL; + + if (!cio2_config) { + printk(BIOS_ERR, "Failed to get CIO2 config\n"); + } else if (cio2_config->device_type != INTEL_ACPI_CAMERA_CIO2) { + printk(BIOS_ERR, "Device type isn't CIO2: %u\n", + (u32)cio2_config->device_type); + } else if (config->ssdb.link_used >= cio2_config->cio2_num_ports) { + printk(BIOS_ERR, "%u exceeds CIO2's %u links\n", + (u32)config->ssdb.link_used, + (u32)cio2_config->cio2_num_ports); + } else { + config->ssdb.lanes_used = + cio2_config->cio2_lanes_used[config->ssdb.link_used]; + } + } + + if (!config->remote_name) { + if (cio2) + config->remote_name = acpi_device_path(cio2); + else + config->remote_name = "\_SB.PCI0.CIO2"; + } }
/* @@ -409,6 +513,9 @@ }
switch (config->device_type) { + case INTEL_ACPI_CAMERA_CIO2: + camera_fill_cio2(dev); + break; case INTEL_ACPI_CAMERA_SENSOR: camera_fill_sensor(dev); break; diff --git a/src/drivers/intel/mipi_camera/chip.h b/src/drivers/intel/mipi_camera/chip.h index 95a3ebb..82223c1 100644 --- a/src/drivers/intel/mipi_camera/chip.h +++ b/src/drivers/intel/mipi_camera/chip.h @@ -127,6 +127,12 @@ const char *chip_name; unsigned int acpi_uid;
+ /* Settings specific to CIO2 device */ + uint32_t cio2_num_ports; + uint32_t cio2_lanes_used[MAX_PORT_ENTRIES]; + const char *cio2_lane_endpoint[MAX_PORT_ENTRIES]; + uint32_t cio2_prt[MAX_PORT_ENTRIES]; + /* Settings specific to camera sensor */ bool disable_ssdb_defaults;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: SSDT changes to handle CIO2 device ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42470/2//COMMIT_MSG@7 PS2, Line 7: drivers/intel/mipi_camera: SSDT changes to handle CIO2 device Please make it a statement by adding a verb (in imperative mood).
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: SSDT changes to handle CIO2 device ......................................................................
Patch Set 2:
(3 comments)
https://review.coreboot.org/c/coreboot/+/42470/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/2/src/drivers/intel/mipi_came... PS2, Line 13: #define CIO2_PCI_DEV 0x14 : #define CIO2_PCI_FN 0x3 nit: tab after CIO2_PCI_...
https://review.coreboot.org/c/coreboot/+/42470/2/src/drivers/intel/mipi_came... PS2, Line 57: ; && j <= MAX_PORT_ENTRIES
https://review.coreboot.org/c/coreboot/+/42470/2/src/drivers/intel/mipi_came... PS2, Line 261: "\_SB.PCI0.CIO2" nit: maybe #define DEFAULT_REMOTE_NAME "\_SB.PCI0.CIO2" ?
Hello build bot (Jenkins), Matt Delco, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42470
to look at the new patch set (#3).
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: Add support to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 115 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42470/3
Hello build bot (Jenkins), Matt Delco, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42470
to look at the new patch set (#4).
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: Add support to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 116 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42470/4
Hello build bot (Jenkins), Matt Delco, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42470
to look at the new patch set (#6).
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: Add support to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 116 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42470/6
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 6:
(4 comments)
https://review.coreboot.org/c/coreboot/+/42470/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/42470/2//COMMIT_MSG@7 PS2, Line 7: drivers/intel/mipi_camera: SSDT changes to handle CIO2 device
Please make it a statement by adding a verb (in imperative mood).
Done
https://review.coreboot.org/c/coreboot/+/42470/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/2/src/drivers/intel/mipi_came... PS2, Line 13: #define CIO2_PCI_DEV 0x14 : #define CIO2_PCI_FN 0x3
nit: tab after CIO2_PCI_...
Done
https://review.coreboot.org/c/coreboot/+/42470/2/src/drivers/intel/mipi_came... PS2, Line 57: ;
&& j <= MAX_PORT_ENTRIES
Done
https://review.coreboot.org/c/coreboot/+/42470/2/src/drivers/intel/mipi_came... PS2, Line 261: "\_SB.PCI0.CIO2"
nit: maybe #define DEFAULT_REMOTE_NAME "\_SB.PCI0. […]
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 6: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/6/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/6/src/drivers/intel/mipi_came... PS6, Line 13: : #define CIO2_PCI_DEV 0x14 : #define CIO2_PCI_FN 0x3 : #define SENSOR_NAME_UUID "822ace8f-2814-4174-a56b-5f029fe079ee" : #define SENSOR_TYPE_UUID "26257549-9271-4ca4-bb43-c4899d5a4881" : #define DEFAULT_REMOTE_NAME "\_SB.PCI0.CIO2" : #define DEFAULT_ENDPOINT 0 nit: line these all up with tabs
Hello build bot (Jenkins), Matt Delco, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42470
to look at the new patch set (#7).
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: Add support to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 116 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42470/7
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/6/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/6/src/drivers/intel/mipi_came... PS6, Line 13: : #define CIO2_PCI_DEV 0x14 : #define CIO2_PCI_FN 0x3 : #define SENSOR_NAME_UUID "822ace8f-2814-4174-a56b-5f029fe079ee" : #define SENSOR_TYPE_UUID "26257549-9271-4ca4-bb43-c4899d5a4881" : #define DEFAULT_REMOTE_NAME "\_SB.PCI0.CIO2" : #define DEFAULT_ENDPOINT 0
nit: line these all up with tabs
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 7: Code-Review+2
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 9:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/9/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/9/src/drivers/intel/mipi_came... PS9, Line 264: if (!config->remote_name) { : if (cio2) : config->remote_name = acpi_device_path(cio2); : else : config->remote_name = DEFAULT_REMOTE_NAME; : } I think a potential issue got introduced during the refactoring. The original code for populating the "remote-endpoint" table had this defaulting of a NULL remote_name be unconditional so that NULL is never passed as part of calling:
acpi_dp_add_reference(remote, NULL, config->remote_name);
The code has now been moved into a camera_fill_sensor_defaults() function that's only called when the unrelated 'config->disable_ssdb_defaults' is false. It seems like this defaulting should occur unconditionally in the parent function (in which case this function can be called 'camera_fill_ssdb_defaults()'), or the checking of 'disable_ssdb_defaults' should instead occur in this function (e.g., do the 'config->remote_name' code first, followed by a 'if (!config->disable_ssdb_defaults) return;' followed by the rest of the function that handles ssdb.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 9: -Code-Review
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/9/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/9/src/drivers/intel/mipi_came... PS9, Line 264: if (!config->remote_name) { : if (cio2) : config->remote_name = acpi_device_path(cio2); : else : config->remote_name = DEFAULT_REMOTE_NAME; : }
I think a potential issue got introduced during the refactoring. […]
Good catch Matt.
Hello Matt Delco, build bot (Jenkins), Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42470
to look at the new patch set (#10).
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: Add support to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 121 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42470/10
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 10:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/9/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/9/src/drivers/intel/mipi_came... PS9, Line 264: if (!config->remote_name) { : if (cio2) : config->remote_name = acpi_device_path(cio2); : else : config->remote_name = DEFAULT_REMOTE_NAME; : }
Good catch Matt.
Done
Hello build bot (Jenkins), Matt Delco, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42470
to look at the new patch set (#11).
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: Add support to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 117 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42470/11
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/11/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/11/src/drivers/intel/mipi_cam... PS11, Line 231: acpi_device_path I should have thought to mention this earlier, but since |config->remote_name| is being modified I'm confused about the lifetime of the data and what, if anything, frees the string (and whether it should be freed).
As things are with this change, |remote_name| can come from 1) being specified in devicetree.cb, 2) coming here from acpi_device_path() (which is returning the address of a static local variable and likely unsafe to store w/o wrapping within strdup()), or 3) DEFAULT_REMOTE_NAME.
Offhand I don't see anyplace where #1 is ever freed (the root |dev_root| is apparently only walked), but if it did I don't know whether this would come from deallocating an entire region or calling free(config->remote_name) (the latter would be an issue since in cases #2 and #3 the underlying memory isn't a dynamic allocation).
If the device tree isn't ever freed, then I suppose it'd be safer to wrap acpi_device_path(cio2) within a strdup() call and never have to worry about freeing it (since the allocation is apparently no worse than if the string had been specified in devicetree.cb).
In the original code change I used a local variable, instead of modifying config->remote_name itself, as a way to avoid trying to find the answer to these questions/issues.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 11:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/11/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/11/src/drivers/intel/mipi_cam... PS11, Line 231: acpi_device_path
I should have thought to mention this earlier, but since |config->remote_name| is being modified I'm confused about the lifetime of the data and what, if anything, frees the string (and whether it should be freed).
The data that is pointed to by `config` lives for all of ramstage, as it a collection of static const data structures (the entire devicetree).
As things are with this change, |remote_name| can come from 1) being specified in devicetree.cb, 2) coming here from acpi_device_path() (which is returning the address of a static local variable and likely unsafe to store w/o wrapping within strdup()), or 3) DEFAULT_REMOTE_NAME.
If there are any other calls to acpi_device_path between here and when the string is emitted in AML, then it will have been "overwritten" so to speak.
Offhand I don't see anyplace where #1 is ever freed (the root |dev_root| is apparently only walked), but if it did I don't know whether this would come from deallocating an entire region or calling free(config->remote_name) (the latter would be an issue since in cases #2 and #3 the underlying memory isn't a dynamic allocation).
Clarifying here, #1 is never freed, it is in the .data section
If the device tree isn't ever freed, then I suppose it'd be safer to wrap acpi_device_path(cio2) within a strdup() call and never have to worry about freeing it (since the allocation is apparently no worse than if the string had been specified in devicetree.cb).
+1, if we're worried about additional calls to acpi_device_path, then that is the safest thing to do.
In the original code change I used a local variable, instead of modifying config->remote_name itself, as a way to avoid trying to find the answer to these questions/issues.
Hello Matt Delco, build bot (Jenkins), Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42470
to look at the new patch set (#12).
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: Add support to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 124 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42470/12
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 12:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/11/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/11/src/drivers/intel/mipi_cam... PS11, Line 231: acpi_device_path
I should have thought to mention this earlier, but since |config->remote_name| is being modified I […]
Done
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 13:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/12/src/drivers/intel/mipi_cam... PS12, Line 87: for (i = 0; i < config->cio2_num_ports; ++i) { : free(ep_table_name[i]); : free(port_table_name[i]); : free(port_name[i]); : } one last thing, so you know it's completely optional to free() memory in coreboot, see the section about memory allocation: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/Doc...
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 13: Code-Review+2
Hello build bot (Jenkins), Matt Delco, Tim Wawrzynczak, Rizwan Qureshi, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/42470
to look at the new patch set (#14).
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: Add support to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 118 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/42470/14
Sugnan Prabhu S has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/12/src/drivers/intel/mipi_cam... PS12, Line 87: for (i = 0; i < config->cio2_num_ports; ++i) { : free(ep_table_name[i]); : free(port_table_name[i]); : free(port_name[i]); : }
one last thing, so you know it's completely optional to free() memory in coreboot, see the section a […]
Done. I have removed them.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 14: Code-Review+2
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/12/src/drivers/intel/mipi_cam... PS12, Line 87: for (i = 0; i < config->cio2_num_ports; ++i) { : free(ep_table_name[i]); : free(port_table_name[i]); : free(port_name[i]); : }
Done. I have removed them.
Hrm, given the style's suggestion to use malloc() as a last resort another option is to use something like:
char ep_table_name[MAX_PORT_ENTRIES][BUS_PATH_MAX] = {""}; char port_table_name[MAX_PORT_ENTRIES][BUS_PATH_MAX] = {""}; char port_name[MAX_PORT_ENTRIES][BUS_PATH_MAX] = {""};
and then delete "char name[BUS_PATH_MAX];" and replace instances like:
snprintf(name, sizeof(name), "EP%u0", i); ep_table_name[i] = strdup(name);
with:
snprintf(ep_table_name[i], sizeof(ep_table_name[i]), "EP%u0", i);
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 14:
(1 comment)
https://review.coreboot.org/c/coreboot/+/42470/12/src/drivers/intel/mipi_cam... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/42470/12/src/drivers/intel/mipi_cam... PS12, Line 87: for (i = 0; i < config->cio2_num_ports; ++i) { : free(ep_table_name[i]); : free(port_table_name[i]); : free(port_name[i]); : }
Hrm, given the style's suggestion to use malloc() as a last resort another option is to use somethin […]
And that alternative uses an extra 384 bytes of stack space... the memory has to come from somewhere either way. I don't think either option is great, it's just an artifact of the current acpi_dp_* API, which already is a heavy malloc() user.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
Patch Set 17: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/42470 )
Change subject: drivers/intel/mipi_camera: Add support to handle CIO2 device ......................................................................
drivers/intel/mipi_camera: Add support to handle CIO2 device
This change updates mipi_camera driver to handle CIO2 device type.
Change-Id: I521740524bc1c4da3d8593f011a033542e4a872c Signed-off-by: Sugnan Prabhu S sugnan.prabhu.s@intel.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/42470 Reviewed-by: Rizwan Qureshi rizwan.qureshi@intel.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 124 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Rizwan Qureshi: Looks good to me, approved
diff --git a/src/drivers/intel/mipi_camera/camera.c b/src/drivers/intel/mipi_camera/camera.c index 7c0774d..03e5ab6 100644 --- a/src/drivers/intel/mipi_camera/camera.c +++ b/src/drivers/intel/mipi_camera/camera.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: GPL-2.0-only */
+#include <stdlib.h> #include <acpi/acpi.h> #include <acpi/acpi_device.h> #include <acpi/acpigen.h> @@ -13,6 +14,82 @@ #define SENSOR_NAME_UUID "822ace8f-2814-4174-a56b-5f029fe079ee" #define SENSOR_TYPE_UUID "26257549-9271-4ca4-bb43-c4899d5a4881" #define DEFAULT_ENDPOINT 0 +#define DEFAULT_REMOTE_NAME "\_SB.PCI0.CIO2" +#define CIO2_PCI_DEV 0x14 +#define CIO2_PCI_FN 0x3 + +/* + * This implementation assumes there is only 1 endpoint at each end of every data port. It also + * assumes there are no clock lanes. It also assumes that any VCM or NVM for a CAM is on the + * same I2C bus as the CAM. + */ + +/* + * Adds settings for a CIO2 device (typically at "_SB.PCI0.CIO2"). A _DSD is added that + * specifies a child table for each port (e.g., PRT0 for "port0" and PRT1 for "port1"). Each + * PRTx table specifies a table for each endpoint (though only 1 endpoint is supported by this + * implementation so the table only has an "endpoint0" that points to a EPx0 table). The EPx0 + * table primarily describes the # of lanes in "data-lines" and specifies the name of the other + * side in "remote-endpoint" (the name is usually of the form "_SB.PCI0.I2Cx.CAM0" for the + * first port/cam and "_SB.PCI0.I2Cx.CAM1" if there's a second port/cam). + */ +static void camera_fill_cio2(const struct device *dev) +{ + struct drivers_intel_mipi_camera_config *config = dev->chip_info; + struct acpi_dp *dsd = acpi_dp_new_table("_DSD"); + char *ep_table_name[MAX_PORT_ENTRIES] = {NULL}; + char *port_table_name[MAX_PORT_ENTRIES] = {NULL}; + char *port_name[MAX_PORT_ENTRIES] = {NULL}; + unsigned int i, j; + char name[BUS_PATH_MAX]; + struct acpi_dp *ep_table = NULL; + struct acpi_dp *port_table = NULL; + struct acpi_dp *remote = NULL; + struct acpi_dp *lanes = NULL; + + for (i = 0; i < config->cio2_num_ports && i < MAX_PORT_ENTRIES; ++i) { + snprintf(name, sizeof(name), "EP%u0", i); + ep_table_name[i] = strdup(name); + ep_table = acpi_dp_new_table(ep_table_name[i]); + acpi_dp_add_integer(ep_table, "endpoint", 0); + acpi_dp_add_integer(ep_table, "clock-lanes", 0); + + if (config->cio2_lanes_used[i] > 0) { + lanes = acpi_dp_new_table("data-lanes"); + + for (j = 1; j <= config->cio2_lanes_used[i] && + j <= MAX_PORT_ENTRIES; ++j) + acpi_dp_add_integer(lanes, NULL, j); + acpi_dp_add_array(ep_table, lanes); + } + + if (config->cio2_lane_endpoint[i]) { + remote = acpi_dp_new_table("remote-endpoint"); + acpi_dp_add_reference(remote, NULL, config->cio2_lane_endpoint[i]); + acpi_dp_add_integer(remote, NULL, 0); + acpi_dp_add_integer(remote, NULL, 0); + acpi_dp_add_array(ep_table, remote); + } + + snprintf(name, sizeof(name), "PRT%u", i); + port_table_name[i] = strdup(name); + port_table = acpi_dp_new_table(port_table_name[i]); + acpi_dp_add_integer(port_table, "port", config->cio2_prt[i]); + acpi_dp_add_child(port_table, "endpoint0", ep_table); + + snprintf(name, sizeof(name), "port%u", i); + port_name[i] = strdup(name); + acpi_dp_add_child(dsd, port_name[i], port_table); + } + + acpi_dp_write(dsd); + + for (i = 0; i < config->cio2_num_ports; ++i) { + free(ep_table_name[i]); + free(port_table_name[i]); + free(port_name[i]); + } +}
static void apply_pld_defaults(struct drivers_intel_mipi_camera_config *config) { @@ -144,11 +221,17 @@ acpigen_pop_len(); /* Method _DSM */ }
-static void camera_fill_sensor_defaults(struct drivers_intel_mipi_camera_config *config) +static void camera_fill_ssdb_defaults(struct drivers_intel_mipi_camera_config *config) { + struct device *cio2 = pcidev_on_root(CIO2_PCI_DEV, CIO2_PCI_FN); + struct drivers_intel_mipi_camera_config *cio2_config; + if (config->disable_ssdb_defaults) return;
+ if (!config->ssdb.bdf_value) + config->ssdb.bdf_value = PCI_DEVFN(CIO2_PCI_DEV, CIO2_PCI_FN); + if (!config->ssdb.platform) config->ssdb.platform = PLATFORM_SKC;
@@ -163,6 +246,24 @@
if (!config->ssdb.mclk_speed) config->ssdb.mclk_speed = CLK_FREQ_19_2MHZ; + + if (!config->ssdb.lanes_used) { + cio2_config = cio2 ? cio2->chip_info : NULL; + + if (!cio2_config) { + printk(BIOS_ERR, "Failed to get CIO2 config\n"); + } else if (cio2_config->device_type != INTEL_ACPI_CAMERA_CIO2) { + printk(BIOS_ERR, "Device type isn't CIO2: %u\n", + (u32)cio2_config->device_type); + } else if (config->ssdb.link_used >= cio2_config->cio2_num_ports) { + printk(BIOS_ERR, "%u exceeds CIO2's %u links\n", + (u32)config->ssdb.link_used, + (u32)cio2_config->cio2_num_ports); + } else { + config->ssdb.lanes_used = + cio2_config->cio2_lanes_used[config->ssdb.link_used]; + } + } }
/* @@ -193,10 +294,12 @@ struct acpi_dp *remote = NULL; const char *vcm_name = NULL; struct acpi_dp *lens_focus = NULL; + const char *remote_name; + struct device *cio2 = pcidev_on_root(CIO2_PCI_DEV, CIO2_PCI_FN);
camera_generate_pld(dev);
- camera_fill_sensor_defaults(config); + camera_fill_ssdb_defaults(config);
/* _DSM */ camera_generate_dsm(dev); @@ -223,7 +326,16 @@
remote = acpi_dp_new_table("remote-endpoint");
- acpi_dp_add_reference(remote, NULL, config->remote_name); + if (config->remote_name) { + remote_name = config->remote_name; + } else { + if (cio2) + remote_name = acpi_device_path(cio2); + else + remote_name = DEFAULT_REMOTE_NAME; + } + + acpi_dp_add_reference(remote, NULL, remote_name); acpi_dp_add_integer(remote, NULL, config->ssdb.link_used); acpi_dp_add_integer(remote, NULL, DEFAULT_ENDPOINT); acpi_dp_add_array(ep00, remote); @@ -408,6 +520,9 @@ }
switch (config->device_type) { + case INTEL_ACPI_CAMERA_CIO2: + camera_fill_cio2(dev); + break; case INTEL_ACPI_CAMERA_SENSOR: camera_fill_sensor(dev); break; diff --git a/src/drivers/intel/mipi_camera/chip.h b/src/drivers/intel/mipi_camera/chip.h index 32b11c1..92d3eac 100644 --- a/src/drivers/intel/mipi_camera/chip.h +++ b/src/drivers/intel/mipi_camera/chip.h @@ -131,6 +131,12 @@ const char *chip_name; unsigned int acpi_uid;
+ /* Settings specific to CIO2 device */ + uint32_t cio2_num_ports; + uint32_t cio2_lanes_used[MAX_PORT_ENTRIES]; + const char *cio2_lane_endpoint[MAX_PORT_ENTRIES]; + uint32_t cio2_prt[MAX_PORT_ENTRIES]; + /* Settings specific to camera sensor */ bool disable_ssdb_defaults;
-
Matt Delco (Code Review) -
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)
-
Rizwan Qureshi (Code Review)
-
Sugnan Prabhu S (Code Review)
-
Tim Wawrzynczak (Code Review)