On 9/19/21 10:07 PM, Sireesh Kodali wrote:
Hi, This RFC patch series adds support for IPA v2, v2.5 and v2.6L (collectively referred to as IPA v2.x).
I'm sorry for the delay on this. I want to give this a reasonable review, but it's been hard to prioritize doing so. So for now I aim to give you some "easy" feedback, knowing that this doesn't cover all issues. This is an RFC, after all... So this isn't a "real review" but I'll try to be helpful. Overall, I appreciate how well you adhered to the patterns and conventions used elsewhere in the driver. There are many levels to that, but I think consistency is a huge factor in keeping code maintainable. I didn't see all that many places where I felt like whining about naming you used, or oddities in indentation, and so on. Abstracting the GSI layer seemed to be done more easily than I expected. I didn't dive deep into the BAM code, and would want to pay much closer attention to that in the future. The BAM/GSI difference is the biggest one dividing IPA v3.0+ from its predecessors. But as you see, the 32- versus 64-bit address and field size differences lead to some ugliness that's hard to avoid. Anyway, nice work; I hope my feedback is helpful. -Alex
Basic description: IPA v2.x is the older version of the IPA hardware found on Qualcomm SoCs. The biggest differences between v2.x and later versions are: - 32 bit hardware (the IPA microcontroler is 32 bit) - BAM (as opposed to GSI as a DMA transport) - Changes to the QMI init sequence (described in the commit message) The fact that IPA v2.x are 32 bit only affects us directly in the table init code. However, its impact is felt in other parts of the code, as it changes the size of fields of various structs (e.g. in the commands that can be sent). BAM support is already present in the mainline kernel, however it lacks two things: - Support for DMA metadata, to pass the size of the transaction from the hardware to the dma client - Support for immediate commands, which are needed to pass commands from the driver to the microcontroller Separate patch series have been created to deal with these (linked in the end) This patch series adds support for BAM as a transport by refactoring the current GSI code to create an abstract uniform API on top. This API allows the rest of the driver to handle DMA without worrying about the IPA version. The final thing that hasn't been touched by this patch series is the IPA resource manager. On the downstream CAF kernel, the driver seems to share the resource code between IPA v2.x and IPA v3.x, which should mean all it would take to add support for resources on IPA v2.x would be to add the definitions in the ipa_data. Testing: This patch series was tested on kernel version 5.13 on a phone with SDM625 (IPA v2.6L), and a phone with MSM8996 (IPA v2.5). The phone with IPA v2.5 was able to get an IP address using modem-manager, although sending/receiving packets was not tested. The phone with IPA v2.6L was able to get an IP, but was unable to send/receive packets. Its modem also relies on IPA v2.6l's compression/decompression support, and without this patch series, the modem simply crashes and restarts, waiting for the IPA block to come up. This patch series is based on code from the downstream CAF kernel v4.9 There are some things in this patch series that would obviously not get accepted in their current form: - All IPA 2.x data is in a single file - Some stray printks might still be around - Some values have been hardcoded (e.g. the filter_map) Please excuse these Lastly, this patch series depends upon the following patches for BAM: [0]: https://lkml.org/lkml/2021/9/19/126 [1]: https://lkml.org/lkml/2021/9/19/135 Regards, Sireesh Kodali Sireesh Kodali (10): net: ipa: Add IPA v2.x register definitions net: ipa: Add support for using BAM as a DMA transport net: ipa: Add support for IPA v2.x commands and table init net: ipa: Add support for IPA v2.x endpoints net: ipa: Add support for IPA v2.x memory map net: ipa: Add support for IPA v2.x in the driver's QMI interface net: ipa: Add support for IPA v2 microcontroller net: ipa: Add IPA v2.6L initialization sequence support net: ipa: Add hw config describing IPA v2.x hardware dt-bindings: net: qcom,ipa: Add support for MSM8953 and MSM8996 IPA Vladimir Lypak (7): net: ipa: Correct ipa_status_opcode enumeration net: ipa: revert to IPA_TABLE_ENTRY_SIZE for 32-bit IPA support net: ipa: Refactor GSI code net: ipa: Establish ipa_dma interface net: ipa: Check interrupts for availability net: ipa: Add timeout for ipa_cmd_pipeline_clear_wait net: ipa: Add support for IPA v2.x interrupts .../devicetree/bindings/net/qcom,ipa.yaml | 2 + drivers/net/ipa/Makefile | 11 +- drivers/net/ipa/bam.c | 525 ++++++++++++++++++ drivers/net/ipa/gsi.c | 322 ++++++----- drivers/net/ipa/ipa.h | 8 +- drivers/net/ipa/ipa_cmd.c | 244 +++++--- drivers/net/ipa/ipa_cmd.h | 20 +- drivers/net/ipa/ipa_data-v2.c | 369 ++++++++++++ drivers/net/ipa/ipa_data-v3.1.c | 2 +- drivers/net/ipa/ipa_data-v3.5.1.c | 2 +- drivers/net/ipa/ipa_data-v4.11.c | 2 +- drivers/net/ipa/ipa_data-v4.2.c | 2 +- drivers/net/ipa/ipa_data-v4.5.c | 2 +- drivers/net/ipa/ipa_data-v4.9.c | 2 +- drivers/net/ipa/ipa_data.h | 4 + drivers/net/ipa/{gsi.h => ipa_dma.h} | 179 +++--- .../ipa/{gsi_private.h => ipa_dma_private.h} | 46 +- drivers/net/ipa/ipa_endpoint.c | 188 ++++--- drivers/net/ipa/ipa_endpoint.h | 6 +- drivers/net/ipa/ipa_gsi.c | 18 +- drivers/net/ipa/ipa_gsi.h | 12 +- drivers/net/ipa/ipa_interrupt.c | 36 +- drivers/net/ipa/ipa_main.c | 82 ++- drivers/net/ipa/ipa_mem.c | 55 +- drivers/net/ipa/ipa_mem.h | 5 +- drivers/net/ipa/ipa_power.c | 4 +- drivers/net/ipa/ipa_qmi.c | 37 +- drivers/net/ipa/ipa_qmi.h | 10 + drivers/net/ipa/ipa_reg.h | 184 +++++- drivers/net/ipa/ipa_resource.c | 3 + drivers/net/ipa/ipa_smp2p.c | 11 +- drivers/net/ipa/ipa_sysfs.c | 6 + drivers/net/ipa/ipa_table.c | 86 +-- drivers/net/ipa/ipa_table.h | 6 +- drivers/net/ipa/{gsi_trans.c => ipa_trans.c} | 182 +++--- drivers/net/ipa/{gsi_trans.h => ipa_trans.h} | 78 +-- drivers/net/ipa/ipa_uc.c | 96 ++-- drivers/net/ipa/ipa_version.h | 12 + 38 files changed, 2133 insertions(+), 726 deletions(-) create mode 100644 drivers/net/ipa/bam.c create mode 100644 drivers/net/ipa/ipa_data-v2.c rename drivers/net/ipa/{gsi.h => ipa_dma.h} (57%) rename drivers/net/ipa/{gsi_private.h => ipa_dma_private.h} (66%) rename drivers/net/ipa/{gsi_trans.c => ipa_trans.c} (80%) rename drivers/net/ipa/{gsi_trans.h => ipa_trans.h} (71%)
