Varshit B Pandya has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Remove unused condition ......................................................................
drivers/intel/mipi_camera: Remove unused condition
NVM also uses PRP0001 as HID hence combining it with condition for VCM
Signed-off-by: Pandya, Varshit B varshit.b.pandya@intel.com Change-Id: Iad7afa7b3170982eb5d6215e766f3e98f7a89213 --- M src/drivers/intel/mipi_camera/camera.c 1 file changed, 3 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/45091/1
diff --git a/src/drivers/intel/mipi_camera/camera.c b/src/drivers/intel/mipi_camera/camera.c index 6f237a9..d126bdd 100644 --- a/src/drivers/intel/mipi_camera/camera.c +++ b/src/drivers/intel/mipi_camera/camera.c @@ -822,10 +822,9 @@
if (config->acpi_hid) acpigen_write_name_string("_HID", config->acpi_hid); - else if (config->device_type == INTEL_ACPI_CAMERA_VCM) - acpigen_write_name_string("_HID", ACPI_DT_NAMESPACE_HID); - else if (config->device_type == INTEL_ACPI_CAMERA_NVM) - acpigen_write_name_string("_HID", "INT3499"); + else if (config->device_type == INTEL_ACPI_CAMERA_VCM || + config->device_type == INTEL_ACPI_CAMERA_NVM) + acpigen_write_name_string("_HID", ACPI_DT_NAMESPACE_HID);
acpigen_write_name_integer("_UID", config->acpi_uid); acpigen_write_name_string("_DDN", config->chip_name);
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Remove unused condition ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45091/1/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/45091/1/src/drivers/intel/mipi_came... PS1, Line 825: else if (config->device_type == INTEL_ACPI_CAMERA_VCM || suspect code indent for conditional statements (8, 24)
Maulik V Vaghela has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Remove unused condition ......................................................................
Patch Set 1: Code-Review+1
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Remove unused condition ......................................................................
Patch Set 1:
I'm not sure what to make of the "unused" claim. Here's a counter-example:
https://review.coreboot.org/c/coreboot/+/31168/1/src/mainboard/google/poppy/...
I'm not clear on whether the scheme for NVM has changed.
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Remove unused condition ......................................................................
Patch Set 1:
Patch Set 1:
I'm not sure what to make of the "unused" claim. Here's a counter-example:
https://review.coreboot.org/c/coreboot/+/31168/1/src/mainboard/google/poppy/...
I'm not clear on whether the scheme for NVM has changed.
"Unused" may not be the correct word. From what I understand looking at the driver code (drivers/misc/eeprom/at24.c), the schemes may be used interchangeably. i.e., HID with INT3499 or a HID with PRP0001 and compatibility with "atmel,24c08"
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Remove unused condition ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
I'm not sure what to make of the "unused" claim. Here's a counter-example: https://review.coreboot.org/c/coreboot/+/31168/1/src/mainboard/google/poppy/...
I'm not clear on whether the scheme for NVM has changed.
"Unused" may not be the correct word. From what I understand looking at the driver code (drivers/misc/eeprom/at24.c), the schemes may be used interchangeably. i.e., HID with INT3499 or a HID with PRP0001 and compatibility with "atmel,24c08"
It does appear as though both are equivalent, though it also looks like almost any of the properties that are set/defaulted based on the ID can be overridden by settings values for ACPI properties "size", "address-width", "read-only", "num-addresses", "no-read-rollover". The main exception seems to be there's no way to set flags for AT24_FLAG_SERIAL, AT24_FLAG_MAC, or AT24_FLAG_IRUGO, though I'm guessing these aren't usually relevant for camera use.
It seems like there's 3 possible schemes: 1) INT3499 with explicit ACPI properties, 2) PRP0001 with a specific compatible string, and 3) scheme #2 plus explicit ACPI properties. To me scheme #3 seems to be a bit redundant and invite inconsistencies with conflicting information, so I'd favor one of the other two schemes. Scheme #1 helps keep consistent with older boards and relies less on whether a OS driver is using appropriate values for a given compat string (it also avoids the need to look up a suitable compat string or deal with a situation where none of the available compat strings matches the characteristics of a new chip).
Anyway, my long-term motivation would be to see Intel's drivers for Linux and Windows converge on a single ACPI scheme, so I'm concerned to see there's now additional schemes going on on the Linux side for advertising NVM and hope there can be at least some kind of consistency/re-convergence on how the NVM is advertised.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Remove unused condition ......................................................................
Patch Set 1:
Patch Set 1:
Patch Set 1:
I'm not sure what to make of the "unused" claim. Here's a counter-example:
https://review.coreboot.org/c/coreboot/+/31168/1/src/mainboard/google/poppy/...
I'm not clear on whether the scheme for NVM has changed.
"Unused" may not be the correct word. From what I understand looking at the driver code (drivers/misc/eeprom/at24.c), the schemes may be used interchangeably. i.e., HID with INT3499 or a HID with PRP0001 and compatibility with "atmel,24c08"
Correct, the kernel can bind through ACPI with a native _HID, or use PRP0001 and then require a "compatible" string for OF matching.
build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Remove unused condition ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/45091/2/src/drivers/intel/mipi_came... File src/drivers/intel/mipi_camera/camera.c:
https://review.coreboot.org/c/coreboot/+/45091/2/src/drivers/intel/mipi_came... PS2, Line 825: else if (config->device_type == INTEL_ACPI_CAMERA_VCM || suspect code indent for conditional statements (8, 24)
Hello build bot (Jenkins), Matt Delco, Maulik V Vaghela, Tim Wawrzynczak, Rizwan Qureshi, Sugnan Prabhu S, Aamir Bohra, Ronak Kanabar, Patrick Rudolph, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45091
to look at the new patch set (#3).
Change subject: drivers/intel/mipi_camera: Add compatible field for NVM ......................................................................
drivers/intel/mipi_camera: Add compatible field for NVM
Add compatible filed for NVM Make PRP0001 as default HID if device type is INTEL_ACPI_CAMERA_NVM
Signed-off-by: Pandya, Varshit B varshit.b.pandya@intel.com Change-Id: Iad7afa7b3170982eb5d6215e766f3e98f7a89213 --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 7 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/45091/3
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Add compatible field for NVM ......................................................................
Patch Set 3:
Patch Set 1:
Patch Set 1:
Patch Set 1:
I'm not sure what to make of the "unused" claim. Here's a counter-example: https://review.coreboot.org/c/coreboot/+/31168/1/src/mainboard/google/poppy/...
I'm not clear on whether the scheme for NVM has changed.
"Unused" may not be the correct word. From what I understand looking at the driver code (drivers/misc/eeprom/at24.c), the schemes may be used interchangeably. i.e., HID with INT3499 or a HID with PRP0001 and compatibility with "atmel,24c08"
It does appear as though both are equivalent, though it also looks like almost any of the properties that are set/defaulted based on the ID can be overridden by settings values for ACPI properties "size", "address-width", "read-only", "num-addresses", "no-read-rollover". The main exception seems to be there's no way to set flags for AT24_FLAG_SERIAL, AT24_FLAG_MAC, or AT24_FLAG_IRUGO, though I'm guessing these aren't usually relevant for camera use.
It seems like there's 3 possible schemes: 1) INT3499 with explicit ACPI properties, 2) PRP0001 with a specific compatible string, and 3) scheme #2 plus explicit ACPI properties. To me scheme #3 seems to be a bit redundant and invite inconsistencies with conflicting information, so I'd favor one of the other two schemes. Scheme #1 helps keep consistent with older boards and relies less on whether a OS driver is using appropriate values for a given compat string (it also avoids the need to look up a suitable compat string or deal with a situation where none of the available compat strings matches the characteristics of a new chip).
To me, I see 1 vs. 2 as just about binding the device to a driver, and if the driver has support for OF already, but not ACPI, it's much simpler to just stick with PRP0001/_DSD until we can get a driver upstreamed.
Anyway, my long-term motivation would be to see Intel's drivers for Linux and Windows converge on a single ACPI scheme, so I'm concerned to see there's now additional schemes going on on the Linux side for advertising NVM and hope there can be at least some kind of consistency/re-convergence on how the NVM is advertised.
I have zero familiarity with the way Windows consumes ACPI, is there some info available on how it looks there?
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Add compatible field for NVM ......................................................................
Patch Set 3:
It seems like there's 3 possible schemes: 1) INT3499 with explicit ACPI properties, 2) PRP0001 with a specific compatible string, and 3) scheme #2 plus explicit ACPI properties. To me scheme #3 seems to be a bit redundant and invite inconsistencies with conflicting information, so I'd favor one of the other two schemes.
To me, I see 1 vs. 2 as just about binding the device to a driver, and if the driver has support for OF already, but not ACPI, it's much simpler to just stick with PRP0001/_DSD until we can get a driver upstreamed.
INT3499 is older and more established (it's what all the poppy-based boards are using) so I'd expect it to have boarder compatibility than PRP0001/_DSD. There's tradeoffs to both, but i mostly wanted people to be aware that there's been a change in precedent that seemed more likely to be accidental (or a reflection of change in personnel) than intentional.
Anyway, my long-term motivation would be to see Intel's drivers for Linux and Windows converge on a single ACPI scheme, so I'm concerned to see there's now additional schemes going on on the Linux side for advertising NVM and hope there can be at least some kind of consistency/re-convergence on how the NVM is advertised.
I have zero familiarity with the way Windows consumes ACPI, is there some info available on how it looks there?
I'm not sure if you're asking about cameras or ACPI in general. Some example DSDT dumps can be found on the net, e.g. (search on cam0/cam1/camf/camr for camera-specific parts):
https://gist.github.com/carlocaione/9c8e2fb47d1100584378 https://gist.github.com/JoshKaufman/e9479620626b10849276443b2875c3c9
and I've looked at other systems that were in a lab (but don't have access to at the moment due to covid-related restrictions).
The cameras basically show up in ACPI as 1 CAM device, with no separate devices for NVM or VCM. The CAM device advertises 1 or more I2C address resources, and custom ACPI attributes in the CAM device help to distinguish which are for the actual camera (vs. optional NVM or VCM). I'm not sure how the NVM type is identified--there's a byte for this in the SSDB blob ("rom_type" in coreboot), but it's very course grained so I figured it was more likely the driver uses I2C to query the NVM device for its make/model. Just to be clear, what I've described in this paragraph is defined by the camera drivers that Intel provides for their SoCs, rather than something defined by Microsoft or the Windows OS.
As for how Windows drivers bind to devices (which is defined by Microsoft), a driver for an ACPI device can basically claim 0+ _HIDs (hardware ID) and/or 0+ _CIDs (compatible ID). Each of those claims can optionally be restricted to a particular _SUB (subsystem ID) (e.g., _HID&_SUB or _CID&_SUB). So, if someone were to write a Windows driver for INT3499 they'd need to write a driver that can handle most/all NVMs, or add a _SUB to the ACPI device and make the driver specific to that _SUB. PRP0001 is used for a much broader range of devices, so the only well-behaved approach would be to use a _SUB (without a _SUB the driver would try to claim all PRP0001 devices on a system, and if it was posted to Windows Update then it could cause havoc on other kinds of systems).
I've used the NVM device as an illustrative example, though as things are currently you wouldn't actually want to write an actual standalone driver for the NVM device. Intel's CAM driver expects to see a monolithic CAM device and will handle the camera, nvm, and vcm functionality together in that driver. If you were to boot Windows on a Chromebook using coreboot and the mipi_camera driver, then it'll see the CAM device, plus possibly an NVM and/or VCM device (depending on the system and devicetree config). The camera can/will work, assuming you've got a driver from Intel for the particular sensor. For the NVM/VCM devices (if present), Windows will likely complain that it doesn't have a driver for the device. A typical way for system manufacturers to mute this complaint is to make a "null driver" (a driver which basically puts a descriptive name on a device but doesn't actually use the device [or even have a presence anywhere in the kernel]--most chipset drivers are really just a null driver). It'd be reasonable for someone to make a null driver for INT3499 (assuming the ID is only used for camera-related NVMs), though handling PRP0001 would be trickier (e.g., add a _SUB on the device and make the driver specific to that).
Rizwan Qureshi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Add compatible field for NVM ......................................................................
Patch Set 3: Code-Review+2
Hello build bot (Jenkins), Matt Delco, Maulik V Vaghela, Rizwan Qureshi, Tim Wawrzynczak, Sugnan Prabhu S, Aamir Bohra, Ronak Kanabar, Patrick Rudolph, Karthikeyan Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/45091
to look at the new patch set (#4).
Change subject: drivers/intel/mipi_camera: Add compatible field for NVM ......................................................................
drivers/intel/mipi_camera: Add compatible field for NVM
Add compatible field for NVM Make PRP0001 as default HID if device type is INTEL_ACPI_CAMERA_NVM
Signed-off-by: Pandya, Varshit B varshit.b.pandya@intel.com Change-Id: Iad7afa7b3170982eb5d6215e766f3e98f7a89213 --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 7 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/91/45091/4
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Add compatible field for NVM ......................................................................
Patch Set 4:
Patch Set 3:
It seems like there's 3 possible schemes: 1) INT3499 with explicit ACPI properties, 2) PRP0001 with a specific compatible string, and 3) scheme #2 plus explicit ACPI properties. To me scheme #3 seems to be a bit redundant and invite inconsistencies with conflicting information, so I'd favor one of the other two schemes.
To me, I see 1 vs. 2 as just about binding the device to a driver, and if the driver has support for OF already, but not ACPI, it's much simpler to just stick with PRP0001/_DSD until we can get a driver upstreamed.
INT3499 is older and more established (it's what all the poppy-based boards are using) so I'd expect it to have boarder compatibility than PRP0001/_DSD. There's tradeoffs to both, but i mostly wanted people to be aware that there's been a change in precedent that seemed more likely to be accidental (or a reflection of change in personnel) than intentional.
Anyway, my long-term motivation would be to see Intel's drivers for Linux and Windows converge on a single ACPI scheme, so I'm concerned to see there's now additional schemes going on on the Linux side for advertising NVM and hope there can be at least some kind of consistency/re-convergence on how the NVM is advertised.
I have zero familiarity with the way Windows consumes ACPI, is there some info available on how it looks there?
I'm not sure if you're asking about cameras or ACPI in general. Some example DSDT dumps can be found on the net, e.g. (search on cam0/cam1/camf/camr for camera-specific parts):
https://gist.github.com/carlocaione/9c8e2fb47d1100584378 https://gist.github.com/JoshKaufman/e9479620626b10849276443b2875c3c9
and I've looked at other systems that were in a lab (but don't have access to at the moment due to covid-related restrictions).
The cameras basically show up in ACPI as 1 CAM device, with no separate devices for NVM or VCM. The CAM device advertises 1 or more I2C address resources, and custom ACPI attributes in the CAM device help to distinguish which are for the actual camera (vs. optional NVM or VCM). I'm not sure how the NVM type is identified--there's a byte for this in the SSDB blob ("rom_type" in coreboot), but it's very course grained so I figured it was more likely the driver uses I2C to query the NVM device for its make/model. Just to be clear, what I've described in this paragraph is defined by the camera drivers that Intel provides for their SoCs, rather than something defined by Microsoft or the Windows OS.
As for how Windows drivers bind to devices (which is defined by Microsoft), a driver for an ACPI device can basically claim 0+ _HIDs (hardware ID) and/or 0+ _CIDs (compatible ID). Each of those claims can optionally be restricted to a particular _SUB (subsystem ID) (e.g., _HID&_SUB or _CID&_SUB). So, if someone were to write a Windows driver for INT3499 they'd need to write a driver that can handle most/all NVMs, or add a _SUB to the ACPI device and make the driver specific to that _SUB. PRP0001 is used for a much broader range of devices, so the only well-behaved approach would be to use a _SUB (without a _SUB the driver would try to claim all PRP0001 devices on a system, and if it was posted to Windows Update then it could cause havoc on other kinds of systems).
I've used the NVM device as an illustrative example, though as things are currently you wouldn't actually want to write an actual standalone driver for the NVM device. Intel's CAM driver expects to see a monolithic CAM device and will handle the camera, nvm, and vcm functionality together in that driver. If you were to boot Windows on a Chromebook using coreboot and the mipi_camera driver, then it'll see the CAM device, plus possibly an NVM and/or VCM device (depending on the system and devicetree config). The camera can/will work, assuming you've got a driver from Intel for the particular sensor. For the NVM/VCM devices (if present), Windows will likely complain that it doesn't have a driver for the device. A typical way for system manufacturers to mute this complaint is to make a "null driver" (a driver which basically puts a descriptive name on a device but doesn't actually use the device [or even have a presence anywhere in the kernel]--most chipset drivers are really just a null driver). It'd be reasonable for someone to make a null driver for INT3499 (assuming the ID is only used for camera-related NVMs), though handling PRP0001 would be trickier (e.g., add a _SUB on the device and make the driver specific to that).
Thanks for the info Matt. If we can keep things working for both OSes, with minimal ugliness, I'm all for that; I was unaware of the reasoning for the way things were w/r/t Windows. I will take a look at the driver and see what we can do.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/45091 )
Change subject: drivers/intel/mipi_camera: Add compatible field for NVM ......................................................................
drivers/intel/mipi_camera: Add compatible field for NVM
Add compatible field for NVM Make PRP0001 as default HID if device type is INTEL_ACPI_CAMERA_NVM
Signed-off-by: Pandya, Varshit B varshit.b.pandya@intel.com Change-Id: Iad7afa7b3170982eb5d6215e766f3e98f7a89213 Reviewed-on: https://review.coreboot.org/c/coreboot/+/45091 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Rizwan Qureshi rizwan.qureshi@intel.com --- M src/drivers/intel/mipi_camera/camera.c M src/drivers/intel/mipi_camera/chip.h 2 files changed, 7 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 6f237a9..fed0ff6 100644 --- a/src/drivers/intel/mipi_camera/camera.c +++ b/src/drivers/intel/mipi_camera/camera.c @@ -454,6 +454,9 @@ 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; @@ -473,6 +476,7 @@ 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); }
@@ -822,10 +826,9 @@
if (config->acpi_hid) acpigen_write_name_string("_HID", config->acpi_hid); - else if (config->device_type == INTEL_ACPI_CAMERA_VCM) + else if (config->device_type == INTEL_ACPI_CAMERA_VCM || + config->device_type == INTEL_ACPI_CAMERA_NVM) acpigen_write_name_string("_HID", ACPI_DT_NAMESPACE_HID); - else if (config->device_type == INTEL_ACPI_CAMERA_NVM) - acpigen_write_name_string("_HID", "INT3499");
acpigen_write_name_integer("_UID", config->acpi_uid); acpigen_write_name_string("_DDN", config->chip_name); diff --git a/src/drivers/intel/mipi_camera/chip.h b/src/drivers/intel/mipi_camera/chip.h index d91e1e7..28f8464 100644 --- a/src/drivers/intel/mipi_camera/chip.h +++ b/src/drivers/intel/mipi_camera/chip.h @@ -241,6 +241,7 @@ uint32_t nvm_pagesize; uint32_t nvm_readonly; uint32_t nvm_width; + const char *nvm_compat;
/* Settings specific to vcm */ const char *vcm_compat;
-
build bot (Jenkins) (Code Review) -
Matt Delco (Code Review)
-
Maulik V Vaghela (Code Review)
-
Patrick Georgi (Code Review)
-
Rizwan Qureshi (Code Review)
-
Tim Wawrzynczak (Code Review)
-
Varshit B Pandya (Code Review)