Re: [RFC PATCH 00/17] net: ipa: Add support for IPA v2.x

[Alex Elder <elder@ieee.org>]

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%)
>