Attention is currently required from: Bora Guvendik, Anil Kumar K, Selma Bensaid, Tim Wawrzynczak, Paul Menzel, Thejaswani Putta. Cliff Huang has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61354 )
Change subject: drivers/wwan/fm: Add Fibocom 5G WWAN ACPI support ......................................................................
Patch Set 2:
(10 comments)
Commit Message:
https://review.coreboot.org/c/coreboot/+/61354/comment/5f66b4a7_11f5b444 PS2, Line 10: when
on
thanks
https://review.coreboot.org/c/coreboot/+/61354/comment/32d3c0be_43893b7a PS2, Line 9: Support PXSX._RST and PXSX.MRST._RST for warm and cold reset. : PXSX._RST is invoked when driver removal. : : Test: : Add chip entry to the corresponding root port and check PXSX Device : is generated in ssdt.
Please do not indent.
sure
Patchset:
PS2:
Thanks Cliff! […]
great. I will create a following CL to move MPTS to here.
File src/drivers/wwan/fm/acpi_fm350gl.c:
https://review.coreboot.org/c/coreboot/+/61354/comment/4652a58f_d69ce464 PS2, Line 29:
Can you add a comment here about what FHRF does?
FHRF: first half reset flow and arg0. I will add the missing header.
https://review.coreboot.org/c/coreboot/+/61354/comment/9bd7d1ff_4a2107d9 PS2, Line 38: assert
`deassert` […]
a bit lost here.... correct me if I am wrong. From rtd3 code. it uses acpigen_enable_tx_gpio to assert reset_gpio pin (i.e. PERST#, also active_low):
_off method: /* Assert reset GPIO to place device into reset. */ if (config->reset_gpio.pin_count) { acpigen_enable_tx_gpio(&config->reset_gpio);
From reading the GPIO value on DUT, acpigen_enable_tx_gpio drive '0' for active_low pin, which means asserting the pin. current code has been working fine for resetting the device, unless I miss a big part...
https://review.coreboot.org/c/coreboot/+/61354/comment/6dbacc53_d0e042ac PS2, Line 42: assert
`deassert` […]
same comment as earlier one.
https://review.coreboot.org/c/coreboot/+/61354/comment/e655da08_161601b1 PS2, Line 44: warm reset
Arg0 == 0, means cold reset and […]
Arg0 = 0: warm reset. Arg1 = 1: cold reset. Let me add to comments.
https://review.coreboot.org/c/coreboot/+/61354/comment/948266d0_5cc4a7f5 PS2, Line 106: acpigen_write_store_int_to_namestr(1, acpi_device_path_join(parent_dev, "RTD3._OFS"));
To ensure that these _ON and _OFF calls are properly paired, I would rather see these incremented an […]
ok. Let me change that.
https://review.coreboot.org/c/coreboot/+/61354/comment/9b2ee31d_7c71e1de PS2, Line 106: acpi_device_path_join(parent_dev, "RTD3._OFS")
nit: this is used twice, extract to a local variable
I will keep the last one.
https://review.coreboot.org/c/coreboot/+/61354/comment/38ef73a6_d152ff5a PS2, Line 167: .read_resources = noop_read_resources, : .set_resources = noop_set_resources, : .acpi_fill_ssdt = wwan_fm350gl_acpi_fill_ssdt, : .acpi_name = wwan_fm350gl_acpi_name, : };
nit: line these up like this (with tabs) […]
ok