Change in coreboot[master]: drivers/wifi: Add CONFIG for WiFi SAR CBFS filename override
Kevin Chiu has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: drivers/wifi: Add CONFIG for WiFi SAR CBFS filename override ......................................................................
drivers/wifi: Add CONFIG for WiFi SAR CBFS filename override
Add CONFIG for variant to override WiFi SAR CBFS filename in sys-boot/coreboot/files/config.X
BUG=b:159304570 BRANCH=master TEST=1. cros-workon-zork start coreboot-private-files-zork 2. emerge-zork chromeos-config coreboot-private-files-zork \ coreboot chromeos-bootimage
Change-Id: Idf859c7bdeb1f41b5144663ba1762e560dcfc789 Signed-off-by: Kevin Chiu kevin.chiu@quantatw.com --- M src/drivers/wifi/generic/Kconfig M src/vendorcode/google/chromeos/sar.c 2 files changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/44672/1
diff --git a/src/drivers/wifi/generic/Kconfig b/src/drivers/wifi/generic/Kconfig index 049a952..c138bda 100644 --- a/src/drivers/wifi/generic/Kconfig +++ b/src/drivers/wifi/generic/Kconfig @@ -47,6 +47,10 @@ depends on WIFI_SAR_CBFS default "src/mainboard/$(MAINBOARDDIR)/wifi_sar_defaults.hex"
+config WIFI_SAR_CBFS_FILENAME + string "The cbfs file name" + default "" + config DSAR_SET_NUM hex "Number of SAR sets when D-SAR is enabled" default 0x3 diff --git a/src/vendorcode/google/chromeos/sar.c b/src/vendorcode/google/chromeos/sar.c index b07d41d..a7263a9 100644 --- a/src/vendorcode/google/chromeos/sar.c +++ b/src/vendorcode/google/chromeos/sar.c @@ -113,5 +113,5 @@ __weak const char *get_wifi_sar_cbfs_filename(void) { - return NULL; + return CONFIG_WIFI_SAR_CBFS_FILENAME; }
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: drivers/wifi: Add CONFIG for WiFi SAR CBFS filename override ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44672/1/src/drivers/wifi/generic/Kc... File src/drivers/wifi/generic/Kconfig:
https://review.coreboot.org/c/coreboot/+/44672/1/src/drivers/wifi/generic/Kc... PS1, Line 50: config WIFI_SAR_CBFS_FILENAME drop this Kconfig.
https://review.coreboot.org/c/coreboot/+/44672/1/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/sar.c:
https://review.coreboot.org/c/coreboot/+/44672/1/src/vendorcode/google/chrom... PS1, Line 116: return CONFIG_WIFI_SAR_CBFS_FILENAME; I was suggesting we should return WIFI_SAR_CBFS_FILENAME here.
Hello build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44672
to look at the new patch set (#2).
Change subject: drivers/wifi: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
drivers/wifi: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source
Each variant WiFi SAR CBFS will be added with the default name "wifi_sar_defaults.hex". so we just need to look up the default CBFS file as the WiFi SAR source.
BUG=b:159304570 BRANCH=master TEST=1. cros-workon-zork start coreboot-private-files-zork 2. emerge-zork chromeos-config coreboot-private-files-zork \ coreboot chromeos-bootimage
Change-Id: Idf859c7bdeb1f41b5144663ba1762e560dcfc789 Signed-off-by: Kevin Chiu kevin.chiu@quantatw.com --- M src/vendorcode/google/chromeos/sar.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/44672/2
Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: drivers/wifi: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44672/1/src/drivers/wifi/generic/Kc... File src/drivers/wifi/generic/Kconfig:
https://review.coreboot.org/c/coreboot/+/44672/1/src/drivers/wifi/generic/Kc... PS1, Line 50: config WIFI_SAR_CBFS_FILENAME
drop this Kconfig.
Done
https://review.coreboot.org/c/coreboot/+/44672/1/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/sar.c:
https://review.coreboot.org/c/coreboot/+/44672/1/src/vendorcode/google/chrom... PS1, Line 116: return CONFIG_WIFI_SAR_CBFS_FILENAME;
I was suggesting we should return WIFI_SAR_CBFS_FILENAME here.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: drivers/wifi: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/44672/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44672/2//COMMIT_MSG@7 PS2, Line 7: drivers/wifi vc/google/chromeos:
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: drivers/wifi: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 2:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44672/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44672/2//COMMIT_MSG@15 PS2, Line 15: master zork
https://review.coreboot.org/c/coreboot/+/44672/2/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/sar.c:
https://review.coreboot.org/c/coreboot/+/44672/2/src/vendorcode/google/chrom... PS2, Line 17: if (filename == NULL) : filename = WIFI_SAR_CBFS_FILENAME; Just a note: With the default implementation of `get_wifi_sar_cbfs_filename()` returning NULL, filename was set to default `WIFI_SAR_CBFS_FILENAME` here already.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: drivers/wifi: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44672/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44672/2//COMMIT_MSG@15 PS2, Line 15: master
zork
It's not there yet, but I think before this change lands, we might have the firmware branch created.
Hello build bot (Jenkins), Furquan Shaikh, Martin Roth, Patrick Georgi, Keith Tzeng, Aaron Durbin,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/44672
to look at the new patch set (#3).
Change subject: vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source
Each variant WiFi SAR CBFS will be added with the default name "wifi_sar_defaults.hex". so we just need to look up the default CBFS file as the WiFi SAR source.
BUG=b:159304570 BRANCH=zork TEST=1. cros-workon-zork start coreboot-private-files-zork 2. emerge-zork chromeos-config coreboot-private-files-zork \ coreboot chromeos-bootimage
Change-Id: Idf859c7bdeb1f41b5144663ba1762e560dcfc789 Signed-off-by: Kevin Chiu kevin.chiu@quantatw.com --- M src/vendorcode/google/chromeos/sar.c 1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/72/44672/3
Kevin Chiu has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/44672/2//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/44672/2//COMMIT_MSG@7 PS2, Line 7: drivers/wifi
vc/google/chromeos:
Done
https://review.coreboot.org/c/coreboot/+/44672/2//COMMIT_MSG@15 PS2, Line 15: master
It's not there yet, but I think before this change lands, we might have the firmware branch created.
Done
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44672/2/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/sar.c:
https://review.coreboot.org/c/coreboot/+/44672/2/src/vendorcode/google/chrom... PS2, Line 17: if (filename == NULL) : filename = WIFI_SAR_CBFS_FILENAME;
Just a note: With the default implementation of `get_wifi_sar_cbfs_filename()` returning NULL, filen […]
Ya. This code is weird. I don't recall the reasoning, but at least the default implementation returns a valid string.
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 3: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 3: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/44672/2/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/sar.c:
https://review.coreboot.org/c/coreboot/+/44672/2/src/vendorcode/google/chrom... PS2, Line 17: if (filename == NULL) : filename = WIFI_SAR_CBFS_FILENAME;
Ya. This code is weird. […]
I think it is probably to allow mainboards to also return NULL to use the default filename.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44672/3/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/sar.c:
https://review.coreboot.org/c/coreboot/+/44672/3/src/vendorcode/google/chrom... PS3, Line 116: return WIFI_SAR_CBFS_FILENAME; We already have ```` const char *filename = get_wifi_sar_cbfs_filename(); if (filename == NULL) filename = WIFI_SAR_CBFS_FILENAME; ````
So what's the purpose of this change?
Aaron Durbin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44672/3/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/sar.c:
https://review.coreboot.org/c/coreboot/+/44672/3/src/vendorcode/google/chrom... PS3, Line 116: return WIFI_SAR_CBFS_FILENAME;
We already have […]
We can skip it if you want. When I was looking things over I thought this would be a decent change, but I didn't interrogate that bit above very closely. Things are a little confusing, but I suppose it doesn't matter much.
Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/44672/3/src/vendorcode/google/chrom... File src/vendorcode/google/chromeos/sar.c:
https://review.coreboot.org/c/coreboot/+/44672/3/src/vendorcode/google/chrom... PS3, Line 116: return WIFI_SAR_CBFS_FILENAME;
We can skip it if you want. […]
I think it's good to have that safe guard (so that implementations of this function can return NULL and things don't explode on us).
We can _also_ make it more explicit here for robustness and clarity (in that it's immediately obvious that this file name is used), I was just curious if there's a particular reason to add that here because I couldn't see anything. I suspect that the code impact is a single address load since the string is already present, with should be ~5 bytes, so *shrug*? :-)
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/44672 )
Change subject: vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source ......................................................................
vc/google/chromeos: load wifi_sar_defaults.hex as the main WiFi SAR CBFS source
Each variant WiFi SAR CBFS will be added with the default name "wifi_sar_defaults.hex". so we just need to look up the default CBFS file as the WiFi SAR source.
BUG=b:159304570 BRANCH=zork TEST=1. cros-workon-zork start coreboot-private-files-zork 2. emerge-zork chromeos-config coreboot-private-files-zork \ coreboot chromeos-bootimage
Change-Id: Idf859c7bdeb1f41b5144663ba1762e560dcfc789 Signed-off-by: Kevin Chiu kevin.chiu@quantatw.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/44672 Reviewed-by: Aaron Durbin adurbin@chromium.org Reviewed-by: Furquan Shaikh furquan@google.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/vendorcode/google/chromeos/sar.c 1 file changed, 1 insertion(+), 1 deletion(-)
Approvals: build bot (Jenkins): Verified Aaron Durbin: Looks good to me, approved Furquan Shaikh: Looks good to me, approved
diff --git a/src/vendorcode/google/chromeos/sar.c b/src/vendorcode/google/chromeos/sar.c index b07d41d..2f73d39 100644 --- a/src/vendorcode/google/chromeos/sar.c +++ b/src/vendorcode/google/chromeos/sar.c @@ -113,5 +113,5 @@ __weak const char *get_wifi_sar_cbfs_filename(void) { - return NULL; + return WIFI_SAR_CBFS_FILENAME; }
-
Aaron Durbin (Code Review) -
Furquan Shaikh (Code Review)
-
Kevin Chiu (Code Review)
-
Patrick Georgi (Code Review)