Add Qualcomm IQ9075 recovery support and improve sparse image handling - #94
Add Qualcomm IQ9075 recovery support and improve sparse image handling#94BakyarajM wants to merge 2 commits into
Conversation
rgantois
left a comment
There was a problem hiding this comment.
Hi, thanks a lot for the contribution! I've requested a few modifications. For the changes in snagflash/android_sparse_file/utils.py, I'll do a more in depth review once you've implemented the formatting changes I've requested. For all other files, no further review should be required once the current requested changes are implemented.
|
|
||
| ### Qualcomm | ||
|
|
||
| #### IQ9075 |
There was a problem hiding this comment.
Since there is only one Qualcomm SoC family supported for now, you can save a title level and just do ### Qualcomm IQ9075
There was a problem hiding this comment.
Done, merged the two heading levels into a single ### Qualcomm IQ9075 heading.
| #### IQ9075 | ||
|
|
||
| To set up the board in recovery mode, refer to the section *"Force the device | ||
| into Emergency Download mode"* in the reference [documentation](https://docs.qualcomm.com/doc/80-70023-261/topic/iq9-ug-update-the-sw.html#panel-0-VWJ1bnR1tab$force-the-device-into-emergency-download-mode) |
There was a problem hiding this comment.
Snagrecover is supposed to be SoC agnostic; of course it's completely fine to add a board-specific example, but you should specify that it's board-specific (in this case for the EVK).
There was a problem hiding this comment.
Added a note clarifying that the reference documentation steps are the same for both EVK and non-EVK boards, except for the DIP switch position — which is EVK-specific and requires checking the board schematic. Other boards should refer to their vendor's documentation for the equivalent step.
|
|
||
| ### For Qualcomm devices | ||
|
|
||
| #### IQ9075 |
There was a problem hiding this comment.
Ditto, you can have a single title level here.
|
|
||
| **xbl:** XBL is a Qualcomm proprietary image, which can be downloaded using the following steps: | ||
|
|
||
| 1. Download the [ZIP](https://softwarecenter.qualcomm.com/nexus/generic/product/chip/tech-package/QCS9100_bootbinaries.1.0/qcs9100_bootbinaries.1.0-test-device-public/00133/QCS9100_bootbinaries.zip) file |
There was a problem hiding this comment.
Is this binary identical for all boards using this SoC? If not, please specify that this binary only works with board xyz and provide a generic description of how to get the binary for other boards such as "request the binary from your board vendor".
There was a problem hiding this comment.
Yes, this XBL binary is generic to the IQ9075 SoC and is identical across all boards using it, so no board-specific note is needed here.
| configuration: | ||
| * path | ||
|
|
||
| **u-boot:** TBD: U-Boot support for Snagboot recovery is currently under upstream review |
There was a problem hiding this comment.
Fine by me, but maybe provide a link to the patch series in question so that people can test it if they want to?
There was a problem hiding this comment.
Added a link to the patch series: https://marc.info/?l=u-boot&m=178577211945398&w=2
| if maxsize == 0: | ||
| raise FastbootError("Fastboot variable max-download-size is 0") | ||
| arg_list = args.split(":") | ||
| arg_list = args.split(":",1) |
There was a problem hiding this comment.
nit: space between function arguments.
|
|
||
| for split_file in split_streaming(fname, temppath, maxsize): | ||
| split_count += 1 | ||
| logger.info(f"Processing split {split_count}: Downloading {split_file}") |
There was a problem hiding this comment.
It would be nice to also have the total number of slices for each log.
There was a problem hiding this comment.
Done, added the total split count to all per-split log/error messages, e.g. "split 3/12".
| # SPDX-License-Identifier: GPL-2.0+ | ||
| # | ||
| # Author: Arnaud Patard <arnaud.patard@collabora.com> | ||
| # |
There was a problem hiding this comment.
Please add a Modified-by: line, similarly to what we did in other files in the codebase. This helps represent the timeline of modifications more accurately.
There was a problem hiding this comment.
However, I could not find any other files in this repository that use a Modified-by: entry. If there are existing examples that follow this convention, could you please point me to them? I may have overlooked them.
| # Reserve space for the trailing DONT_CARE suffix chunk header | ||
| _SUFFIX_RESERVE = SPARSE_CHUNKHEADER_LEN | ||
|
|
||
| def ensure_prefix_skip(): |
There was a problem hiding this comment.
Please don't use inline function definitions, it adds unnecessary indentation levels and makes things harder to read IMO.
There was a problem hiding this comment.
Done, removed the inline nested functions and replaced them with module-level functions using an explicit state object instead of closures.
|
|
||
| return dest | ||
|
|
||
| try: |
There was a problem hiding this comment.
Please factor this part into multiple functions, currently it's quite hard to review.
There was a problem hiding this comment.
Done, factored the chunk-processing loop into three separate functions, one per chunk type (process_raw_chunk, process_dontcare_chunk, process_fill_chunk). split_streaming() now just orchestrates the dispatch between them.
Add support for Qualcomm IQ9075 in snagrecover. Implement Qualcomm Sahara protocol support to communicate with Qualcomm devices in EDL mode and perform device recovery. Reference: https://github.com/torvalds/linux/blob/master/drivers/accel/qaic/sahara.c Signed-off-by: Bakyaraj Moorthy <bmoorthy@qti.qualcomm.com>
Handle sparse chunks larger than max-download-size and preserve DONT_CARE prefix and suffix regions when splitting sparse images. Ignore CRC32 chunks from the source sparse image during splitting, as the original CRC32 is no longer valid once the image is split or modified. Fix fastboot flash argument parsing for UFS flash type by splitting only on the first ':' character, preserving target identifiers in the format <lun>:<partition> while maintaining compatibility with other flash types. Replace the existing split() implementation with the streaming-based split_streaming() generator to enable more memory-efficient processing of large sparse images. Future enhancements: - Generate and append a new CRC32 chunk for each split sparse image based on its modified content. - Convert raw images to sparse format and flash them as multiple sparse images when the image size exceeds the target's max-download-size limit. Reference: https://android.googlesource.com/platform/system/core/+/refs/heads/main/libsparse/ Signed-off-by: Bakyaraj Moorthy <bmoorthy@qti.qualcomm.com>
35aa3e4 to
407669e
Compare
|
|
Hi, thanks for the detailed review! |
snagrecover: qcom: Add IQ9075 support
Add support for Qualcomm IQ9075 devices in snagrecover.
Implement Qualcomm Sahara protocol support to communicate with
Qualcomm devices in EDL mode and perform device recovery.
Reference:
https://github.com/torvalds/linux/blob/master/drivers/accel
_Note:
The U-Boot changes required to support snagrecover are currently
under review. Therefore, the U-Boot image preparation section in
docs/snagrecover.md is marked as TBD.
The documentation will be updated and the TBD item resolved before
the final upstream submission._
snagflash: Improve sparse file handling
Handle sparse chunks larger than max-download-size and preserve
DONT_CARE prefix and suffix regions when splitting sparse images.
Ignore CRC32 chunks from the source sparse image during splitting,
as the original CRC32 is no longer valid once the image is split or
modified.
Fix fastboot flash argument parsing for UFS flash type by splitting
only on the first ':' character, preserving target identifiers in
the format : while maintaining compatibility with
other flash types.
Replace the existing split() implementation with the streaming-based
split_streaming() generator to enable more memory-efficient
processing of large sparse images.
_Future enhancements:
based on its modified content.
sparse images when the image size exceeds the target's
max-download-size limit._
Reference:
https://android.googlesource.com/platform/system/core/+/refs/heads/main/libsparse/
Signed-off-by: Bakyaraj Moorthy bmoorthy@qti.qualcomm.com