mbox series

[v2,00/17] net: introduce Qualcomm IPA driver (UPDATED)

Message ID 20200306042831.17827-1-elder@linaro.org (mailing list archive)
Headers show
Series net: introduce Qualcomm IPA driver (UPDATED) | expand

Message

Alex Elder March 6, 2020, 4:28 a.m. UTC
This series presents the driver for the Qualcomm IP Accelerator (IPA).

This is version 2 of this updated series.  It includes the following
small changes since the previous version:
  - Now based on net-next instead of v5.6-rc
  - Config option now named CONFIG_QCOM_IPA
  - Some minor cleanup in the GSI code
  - Small change to replenish logic
  - No longer depends on remoteproc bug fixes
What follows is the basically same explanation as was posted previously.

					-Alex

I have posted earlier versions of this code previously, but it has
undergone quite a bit of development since the last time, so rather
than calling it "version 3" I'm just treating it as a new series
(indicating it's been updated in this message).  The fast/data path
is the same as before.  But the driver now (nearly) supports a
second platform, its transaction handling has been generalized
and improved, and modem activities are now handled in a more
unified way.

This series is available (based on net-next in branch "ipa_updated-v2"
in this git repository:
  https://git.linaro.org/people/alex.elder/linux.git

The branch depends on other one other small patch that I sent out
for review earlier.
  https://lore.kernel.org/lkml/20200306042302.17602-1-elder@linaro.org/


I want to address some of the discussion that arose last time.

First, there was the WWAN discussion.  Here's the history:
  - This was last posted nine months ago.
  - Reviewers at that time favored developing a new WWAN subsystem that
    would be used for managing devices like this.  And the suggestion
    was to not accept this driver until that could be developed.
  - Along the way, Apple acquired much of Intel's modem business.
    And as a result, the generic framework became less pressing.
  - I did participate in the WWAN subsystem design however, and
    although it went dormant for a while it's been resurrected:
      https://lore.kernel.org/netdev/20200225100053.16385-1-johannes@sipsolutions.net/
  - Unfortunately the proposed WWAN design was not an easy fit
    with Qualcomm's integrated modem interfaces.  Given that
    rmnet is a supported link type for in the upstream "iproute2"
    package (more on this below), I have opted not to integrate
    with any WWAN subsystem.

So in summary, this driver does not integrate with a generic WWAN
framework.  And I'd like it to be accepted upstream despite that.


Next, Arnd Bergmann had some concerns about flow control.  (Note:
some of my discussions with Arnd about this were offline.) The
overall architecture here also involves the "rmnet" driver:
  drivers/net/ethernet/qualcomm/rmnet

The rmnet driver presents a network device for use.  It connects
with another network device presented, by the IPA driver.  The
rmnet driver wraps (and unwraps) packets transferred to (and from)
the IPA driver with QMAP headers.

   ---------------
   | rmnet_data0 |    <-- "real" netdev
   ---------------
          ||       }- QMAP spoken here
   --------------
   | rmnet_ipa0 |     <-- also netdev, transporting QMAP packets
   --------------
          ||
   --------------
  ( IPA hardware )
   --------------

Arnd's concern was that the rmnet_data0 network device does not
have the benefit of information about the state of the underlying
IPA hardware in order to be effective in controlling TX flow.
The feared result is over-buffering of TX packets (bufferbloat).
I began working on some simple experiments to see whether (or how
much) his concern was warranted.  But it turned out that completing
these experiments was much more work than had been hoped.

The rmnet driver is present in the upstream kernel.  There is also
support for the rmnet link type in the upstream "ip" user space
command in the "iproute2" package.  Changing the layering of rmnet
over IPA likely involves deprecating the rmnet driver and its
support in "iproute2".  I would really rather not go down that
path.

There is precedent for this sort of layering of network devices
(L2TP, VLAN).  And any architecture like this would suffer the
issues Arnd mentioned; the problem is not limited to rmnet and IPA.
I do think this is a problem worth solving, but the prudent thing
to do might be to try to solve it more generally.

So to summarize on this issue, this driver does not attempt to
change the way the rmnet and IPA drivers work together.  And even
though I think Arnd's concerns warrant more investigation, I'd like
this driver to to be accepted upstream without any change to this
architecture.


Finally, a more technical description for the series, and some
acknowledgements to some people who contributed to it.

The IPA is a component present in some Qualcomm SoCs that allows
network functions such as aggregation, filtering, routing, and NAT
to be performed without active involvement of the main application
processor (AP).

In this initial patch series these advanced features are not
implemented.  The IPA driver simply provides a network interface
that makes the modem's LTE network available in Linux.  This initial
series supports only the Qualcomm SDM845 SoC.  The Qualcomm SC7180
SoC is partially supported, and support for other platforms will
follow.

This code is derived from a driver developed by Qualcomm.  A version
of the original source can be seen here:
  https://source.codeaurora.org/quic/la/kernel/msm-4.9/tree
in the "drivers/platform/msm/ipa" directory.  Many were involved in
developing this, but the following individuals deserve explicit
acknowledgement for their substantial contributions:

    Abhishek Choubey
    Ady Abraham
    Chaitanya Pratapa
    David Arinzon
    Ghanim Fodi
    Gidon Studinski
    Ravi Gummadidala
    Shihuan Liu
    Skylar Chang

					-Alex

Alex Elder (17):
  remoteproc: add IPA notification to q6v5 driver
  dt-bindings: soc: qcom: add IPA bindings
  soc: qcom: ipa: main code
  soc: qcom: ipa: configuration data
  soc: qcom: ipa: clocking, interrupts, and memory
  soc: qcom: ipa: GSI headers
  soc: qcom: ipa: the generic software interface
  soc: qcom: ipa: IPA interface to GSI
  soc: qcom: ipa: GSI transactions
  soc: qcom: ipa: IPA endpoints
  soc: qcom: ipa: filter and routing tables
  soc: qcom: ipa: immediate commands
  soc: qcom: ipa: modem and microcontroller
  soc: qcom: ipa: AP/modem communications
  soc: qcom: ipa: support build of IPA code
  MAINTAINERS: add entry for the Qualcomm IPA driver
  arm64: dts: sdm845: add IPA information

 .../devicetree/bindings/net/qcom,ipa.yaml     |  192 ++
 MAINTAINERS                                   |    6 +
 arch/arm64/boot/dts/qcom/sdm845.dtsi          |   51 +
 drivers/net/Kconfig                           |    2 +
 drivers/net/Makefile                          |    1 +
 drivers/net/ipa/Kconfig                       |   19 +
 drivers/net/ipa/Makefile                      |   12 +
 drivers/net/ipa/gsi.c                         | 2055 +++++++++++++++++
 drivers/net/ipa/gsi.h                         |  257 +++
 drivers/net/ipa/gsi_private.h                 |  118 +
 drivers/net/ipa/gsi_reg.h                     |  417 ++++
 drivers/net/ipa/gsi_trans.c                   |  786 +++++++
 drivers/net/ipa/gsi_trans.h                   |  226 ++
 drivers/net/ipa/ipa.h                         |  148 ++
 drivers/net/ipa/ipa_clock.c                   |  313 +++
 drivers/net/ipa/ipa_clock.h                   |   53 +
 drivers/net/ipa/ipa_cmd.c                     |  680 ++++++
 drivers/net/ipa/ipa_cmd.h                     |  195 ++
 drivers/net/ipa/ipa_data-sc7180.c             |  307 +++
 drivers/net/ipa/ipa_data-sdm845.c             |  329 +++
 drivers/net/ipa/ipa_data.h                    |  280 +++
 drivers/net/ipa/ipa_endpoint.c                | 1707 ++++++++++++++
 drivers/net/ipa/ipa_endpoint.h                |  110 +
 drivers/net/ipa/ipa_gsi.c                     |   54 +
 drivers/net/ipa/ipa_gsi.h                     |   60 +
 drivers/net/ipa/ipa_interrupt.c               |  253 ++
 drivers/net/ipa/ipa_interrupt.h               |  117 +
 drivers/net/ipa/ipa_main.c                    |  954 ++++++++
 drivers/net/ipa/ipa_mem.c                     |  314 +++
 drivers/net/ipa/ipa_mem.h                     |   90 +
 drivers/net/ipa/ipa_modem.c                   |  383 +++
 drivers/net/ipa/ipa_modem.h                   |   31 +
 drivers/net/ipa/ipa_qmi.c                     |  538 +++++
 drivers/net/ipa/ipa_qmi.h                     |   41 +
 drivers/net/ipa/ipa_qmi_msg.c                 |  663 ++++++
 drivers/net/ipa/ipa_qmi_msg.h                 |  252 ++
 drivers/net/ipa/ipa_reg.c                     |   38 +
 drivers/net/ipa/ipa_reg.h                     |  476 ++++
 drivers/net/ipa/ipa_smp2p.c                   |  335 +++
 drivers/net/ipa/ipa_smp2p.h                   |   48 +
 drivers/net/ipa/ipa_table.c                   |  700 ++++++
 drivers/net/ipa/ipa_table.h                   |  103 +
 drivers/net/ipa/ipa_uc.c                      |  211 ++
 drivers/net/ipa/ipa_uc.h                      |   32 +
 drivers/net/ipa/ipa_version.h                 |   23 +
 drivers/remoteproc/Kconfig                    |    6 +
 drivers/remoteproc/Makefile                   |    1 +
 drivers/remoteproc/qcom_q6v5_ipa_notify.c     |   85 +
 drivers/remoteproc/qcom_q6v5_mss.c            |   38 +
 .../linux/remoteproc/qcom_q6v5_ipa_notify.h   |   82 +
 50 files changed, 14192 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/qcom,ipa.yaml
 create mode 100644 drivers/net/ipa/Kconfig
 create mode 100644 drivers/net/ipa/Makefile
 create mode 100644 drivers/net/ipa/gsi.c
 create mode 100644 drivers/net/ipa/gsi.h
 create mode 100644 drivers/net/ipa/gsi_private.h
 create mode 100644 drivers/net/ipa/gsi_reg.h
 create mode 100644 drivers/net/ipa/gsi_trans.c
 create mode 100644 drivers/net/ipa/gsi_trans.h
 create mode 100644 drivers/net/ipa/ipa.h
 create mode 100644 drivers/net/ipa/ipa_clock.c
 create mode 100644 drivers/net/ipa/ipa_clock.h
 create mode 100644 drivers/net/ipa/ipa_cmd.c
 create mode 100644 drivers/net/ipa/ipa_cmd.h
 create mode 100644 drivers/net/ipa/ipa_data-sc7180.c
 create mode 100644 drivers/net/ipa/ipa_data-sdm845.c
 create mode 100644 drivers/net/ipa/ipa_data.h
 create mode 100644 drivers/net/ipa/ipa_endpoint.c
 create mode 100644 drivers/net/ipa/ipa_endpoint.h
 create mode 100644 drivers/net/ipa/ipa_gsi.c
 create mode 100644 drivers/net/ipa/ipa_gsi.h
 create mode 100644 drivers/net/ipa/ipa_interrupt.c
 create mode 100644 drivers/net/ipa/ipa_interrupt.h
 create mode 100644 drivers/net/ipa/ipa_main.c
 create mode 100644 drivers/net/ipa/ipa_mem.c
 create mode 100644 drivers/net/ipa/ipa_mem.h
 create mode 100644 drivers/net/ipa/ipa_modem.c
 create mode 100644 drivers/net/ipa/ipa_modem.h
 create mode 100644 drivers/net/ipa/ipa_qmi.c
 create mode 100644 drivers/net/ipa/ipa_qmi.h
 create mode 100644 drivers/net/ipa/ipa_qmi_msg.c
 create mode 100644 drivers/net/ipa/ipa_qmi_msg.h
 create mode 100644 drivers/net/ipa/ipa_reg.c
 create mode 100644 drivers/net/ipa/ipa_reg.h
 create mode 100644 drivers/net/ipa/ipa_smp2p.c
 create mode 100644 drivers/net/ipa/ipa_smp2p.h
 create mode 100644 drivers/net/ipa/ipa_table.c
 create mode 100644 drivers/net/ipa/ipa_table.h
 create mode 100644 drivers/net/ipa/ipa_uc.c
 create mode 100644 drivers/net/ipa/ipa_uc.h
 create mode 100644 drivers/net/ipa/ipa_version.h
 create mode 100644 drivers/remoteproc/qcom_q6v5_ipa_notify.c
 create mode 100644 include/linux/remoteproc/qcom_q6v5_ipa_notify.h

Comments

David Miller March 9, 2020, 5:09 a.m. UTC | #1
From: Alex Elder <elder@linaro.org>
Date: Thu,  5 Mar 2020 22:28:14 -0600

> This series presents the driver for the Qualcomm IP Accelerator (IPA).

Series applied, thank you.
Dave Taht March 9, 2020, 4:54 p.m. UTC | #2
I am happy to see this driver upstream.

>Arnd's concern was that the rmnet_data0 network device does not
>have the benefit of information about the state of the underlying
>IPA hardware in order to be effective in controlling TX flow.
>The feared result is over-buffering of TX packets (bufferbloat).
>I began working on some simple experiments to see whether (or how
>much) his concern was warranted.  But it turned out that completing
>these experiments was much more work than had been hoped.

Members of the bufferbloat project *care*, and have tools and testbeds for
exploring these issues. It would be good to establish a relationship with
the vendor, obtain hardware, and other (technical and financial) support, if
possible.

Is there any specific hardware now available (generally or in beta) that
can be obtained by us to take a harder look? A contact at linaro or QCA
willing discuss options?
Alex Elder March 12, 2020, 3:09 a.m. UTC | #3
On 3/9/20 11:54 AM, Dave Taht wrote:
> I am happy to see this driver upstream.
> 
>> Arnd's concern was that the rmnet_data0 network device does not
>> have the benefit of information about the state of the underlying
>> IPA hardware in order to be effective in controlling TX flow.
>> The feared result is over-buffering of TX packets (bufferbloat).
>> I began working on some simple experiments to see whether (or how
>> much) his concern was warranted.  But it turned out that completing
>> these experiments was much more work than had been hoped.
> 
> Members of the bufferbloat project *care*, and have tools and testbeds for
> exploring these issues. It would be good to establish a relationship with
> the vendor, obtain hardware, and other (technical and financial) support, if
> possible.
> 
> Is there any specific hardware now available (generally or in beta) that
> can be obtained by us to take a harder look? A contact at linaro or QCA
> willing discuss options?

There exists some hardware that could be used, but at the moment I have
not ported this code to operate on it.  It is a current effort however,
and I will be glad to keep you in the loop on progress.  There are a
couple of target environments we'd like to support but until last week
the primary goal was inclusion in the upstream tree.

I will follow up with you after the dust settles a little bit with
this patch series, maybe in a week or so.  In the mean time I'll
also find out whether there are any other resources (people and/or
hardware) available.

					-Alex
Evan Green April 29, 2020, 11:17 p.m. UTC | #4
On Thu, Mar 5, 2020 at 8:28 PM Alex Elder <elder@linaro.org> wrote:
>
> This series presents the driver for the Qualcomm IP Accelerator (IPA).
>
> This is version 2 of this updated series.  It includes the following
> small changes since the previous version:
>   - Now based on net-next instead of v5.6-rc
>   - Config option now named CONFIG_QCOM_IPA
>   - Some minor cleanup in the GSI code
>   - Small change to replenish logic
>   - No longer depends on remoteproc bug fixes
> What follows is the basically same explanation as was posted previously.
>
>                                         -Alex
>
> I have posted earlier versions of this code previously, but it has
> undergone quite a bit of development since the last time, so rather
> than calling it "version 3" I'm just treating it as a new series
> (indicating it's been updated in this message).  The fast/data path
> is the same as before.  But the driver now (nearly) supports a
> second platform, its transaction handling has been generalized
> and improved, and modem activities are now handled in a more
> unified way.
>
> This series is available (based on net-next in branch "ipa_updated-v2"
> in this git repository:
>   https://git.linaro.org/people/alex.elder/linux.git
>
> The branch depends on other one other small patch that I sent out
> for review earlier.
>   https://lore.kernel.org/lkml/20200306042302.17602-1-elder@linaro.org/
>

I realize this is all already in (yay!), but it took me a long time to
get around to fully reading this driver. I'll paste my notes here for
posterity or possible future patches. Overall the driver seemed well
documented and thoughtfully written. As someone who has seen the old
downstream IPA driver (though I didn't look long as my brain started
hurting), I greatly appreciate the work required by Alex to polish
this all up. So firstly, thanks Alex!

Onto the notes. There are a couple themes I noticed. The driver seems
occasionally to be unnecessarily layer-caked. I noticed "could be
inlined" as a common refrain in my feedback. There are also a couple
places with hand-rolled refcounting, atomic exchanges, and odd
mutexes. I haven't fully digested those to be able to know how to get
rid of them, but I'll point them out as something that "doesn't smell
quite right".

Acronyms (for my own benefit):
ee - execution environment
ep - endpoint
er - endpoint or route ID
rt - resource type
dcd - Dynamic clock division (request to GCC to turn you off)
bcr - Backwards compatibility register
comp - Core master port
holb - ???

ipa_main.c:
What is IPA_VALIDATION. Can this just be on always or removed?
otherwise it will likely bit rot.
I'd like to see this suspend_ref go away.
ipa_reg.c can be inlined
ipa_mem_init can be inlined.


IPA_NOTIFY:
Shouldn't CONFIG_IPA depend on IPA_NOTIFY?


ipa_data.h
Why are ipa_resource_src and ipa_resource_dst separate structures?
maybe the extern globals at the bottom should just be moved into ipa_main.c


ipa_endpoint.h
Add a note for enum ipa_endpoint_name indicating who is TXing and RXing


ipa_data-sc7180.c
Where is IPA_ENDPOINT_MODEM_LAN_TX definition?


ipa_clock.c
IPA_CORE_CLOCK_RATE - Should probably be specified in DT as a fixed
frequency rather than here in code.
Interconnect bandwidths - Are these a function of the core clock rate?
This may be fine for the initial version, but is there any way to
derive the bandwidth requirement?
ipa_interconnect_init_one - Probably best to just inline this
ipa_clock_get_additional - Seems sketchy, would like to remove this
Overall don't like the homebrew reference counting here. Would runtime
PM help you do this?


ipa_interrupt.h
I'd like to get rid of ipa_interrupt_add and ipa_interrupt_remove.
Seems like there's no need for these to be dynamically added, it's all
one driver.


ipa_interrupt.c
Why does ipa_interrupt_setup() need to dynamically allocate the
structure, can't we just embed it in struct ipa?
Without the kzalloc, ipa_interrupt_setup() and
ipa_interrupt_teardown() are simple enough they can probably be
inlined (at least teardown for sure).
Interrupt processing seems a little odd. What I would have expected is:
Hard ISR reads pending bits, and immediately writes all pending bits
to quiesce them. Save bitmask of pending bits, and send to the
threaded handler. Threaded handler then reads and clears pending bits
out, and acts on any.
Fixes interrupt storm in ipa_isr() if an unexpected interrupt comes in
but an expected interrupt is also pending.
Avoids multiple register writes (one for each bit) in ipa_interrupt_process()
Saves all the register reads in ipa_interrupt_process_all(). That
additional read in the loop seems like it shouldn't be there either
way.


ipa_mem.h
Is IPA_SHARED_MEM_SIZE supposed to be defined? It's mentioned in the comment.
Comment says the number of canaries is the same for all IPA versions,
but ipa_data-sdm845.c and ipa_data-sc7180.c seem to have different
canary counts for IPA_MEM_UC_INFO?
Should the number of canaries really be part of the chipset-specific
config info if it's never going to change?
Do the canary values eat into the previous region? Can we add a
warning to ensure we don't write canary values off the beginning of
the memory region?


ipa_mem.c
Maybe remove ipa_mem_teardown() if we're not planning to add anything
to it soon, or inline it in the header for now.
Does ipa_mem_zero_modem() erase canary values previously set up?


gsi.h
Why make gsi_evt_ring_state 0xf? Remove assignments and let enum do its thing.
enum gsi_ee_id - Probably worth commenting that this defines the
layout of the per-EE register regions, so rearranging this would
horribly break our access to hardware.


gsi_reg.h
What is gsi v2.0? Is that the same as IPA 4.0?
Why do the channel macros have things like CH_C and EE_N in them? Why
not just CH and EE? Oh, I also see CH_E, what's that?


gsi.c:
enum gsi_err_code: Where's 0x7?
gsi_channel_deprogram(): delete
gsi_channel_update(): I'm worried about this refcount thing, how does it work?
gsi_event_bitmap_init() can be inlined
gsi_evt_ring_setup() and gsi_evt_ring_teardown() can be removed
gsi_teardown(): inline
gsi_evt_ring_exit(): remove


ipa_gsi.h:
Comment for ipa_gsi_channel_tx_completed has wrong function name copypasta.


ipa_gsi.c:
This is an interesting mezzanine interface, it looks like it was
designed to keep GSI code from calling IPA code directly. Why is that?
Could these at least be inlined into the ipa_gsi.h?


gsi_trans.h:
Why is it important that struct gsi_trans be < 128 bytes?


gsi_trans.c:
gsi_tre_type - Should this be in a header?
TRE_FLAGS_ - Should these be in a header? Also, replace GENMASK(x,x)
with BIT(x). TRE_FLAGS_IEOB_FMASK is never used (which is fine, but
should it be?)
gsi_trans_tre_reserve() - Why atomic_try_cmpxchg? What's the
difference between that and atomic_cmpxchg?
gsi_tre_len_opcode() - If len is truncated to 16 bits, why is u32
passed in? Is len sometimes used as 32 bits?
gsi_trans_tre_fill() - If it doesn't do a 16-byte atomic write, is
this a problem? Could the controller see a half-baked TRE?


ipa_endpoint.c:
What is HOLB timer?


ipa_table.c:
ipa_table_valid() - This just runs all 3-bit possibilities. Could use
flags and a loop instead.
ipa_table_teardown() - Remove?


ipa_cmd.c:
ipa_cmd_tag_process_add() - What happened here? Is this just
functionality we're not using right now?


ipa_modem.c
ipa_start_xmit() - Could returning BUSY result in an infinite loop if
something goes wrong in the lower layers?
ipa_modem_start() - Shouldn't we print some errors if the state
variable has an unexpected value (ie not RUNNING)? In those cases we
are likely not in a good place.


ipa_qmi.c:
ipa_qmi_indication() could be inlined
init_modem_driver_req() use of static means this can never run
concurrently with itself, right? Also if the request gets stuck in
qmi_txn_wait() you're hosed.


ipa_qmi_msg.c
You could macro-ize the initialization of these elements, which would
make things way shorter, and probably easier to read. I'm imagining
for instance the first element in the file could be reduced to
IPA_QMI_ELEM(QMI_OPT_FLAG, 1, struct ipa_indication_register_req,
master_driver_init_complete_valid, 0x10)


ipa_smp2p.c:
s/Motex/Mutex/
Actually I don't get why the mutex is needed at all. It's certainly
not needed in ipa_smp2p_disable() (stores are already atomic), and
threaded irqs already have mutual exclusion. Or are you trying to make
sure ipa_smp2p_disable() doesn't return until
ipa_smp2p_modem_setup_ready_isr() has fully completed? If that's
really why, you should explain that's what it's doing and why it's
necessary.
Thinking more about it, why can't you just actually disable the irq?
That calls synchronize_irq, which will flush out any instances of the
irq running. Then no mutex necessary!
ipa_smp2p_irq_init(), and _exit() can be inlined.
I'd love to see clock_on and the weird reference counting go away. Is
that really necessary?