GitLab

In review for commit 8ee7ec48

 ("net: ipa: embed interconnect
array in the power structure"), Jakub Kicinski suggested that a
follow-up patch use struct_size() when computing the size of the
IPA power structure, which ends with a flexible array member.

Do that.
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Alex Elder <elder@linaro.org>
Link: https://lore.kernel.org/r/20220311162423.872645-1-elder@linaro.org

Signed-off-by: Jakub Kicinski <kuba@kernel.org>

The ipa_power structure contains a copy of the IPA device pointer,
so there's no need to pass it to ipa_interconnect_init().  We can
also use that pointer for an error message in ipa_power_enable().
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Rather than allocating the interconnect array dynamically, represent
the interconnects with a variable-length array at the end of the
ipa_power structure.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

The previous patch used bulk interconnect operations to initialize
IPA interconnects one at a time.  This rearranges things to use the
bulk interfaces as intended--on all interconnects together.  As a
result ipa_interconnect_init_one() and ipa_interconnect_exit_one()
are no longer needed.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Use of_icc_bulk_get() and icc_bulk_put(), icc_bulk_set_bw(), and
icc_bulk_enable() and icc_bulk_disable() to initialize individual
IPA interconnects.  Those functions already log messages in the
event of error so we don't need to.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

The power interconnect array is now an array of icc_bulk_data
structures, which is what the interconnect bulk enable and disable
functions require.

Get rid of ipa_interconnect_enable() and ipa_interconnect_disable(),
and just call icc_bulk_enable() and icc_bulk_disable() instead.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

The interconnect framework now provides the ability to enable and
disable interconnects without having to change their recorded
"enabled" bandwidth value.  Use this mechanism, rather than setting
the bandwidth values to zero and non-zero respectively to disable
and enable the IPA interconnects.

Disable each interconnect before setting its "enabled" average and
peak bandwidth values.  Thereafter, enable and disable interconnects
when required rather than setting their bandwidths.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

The ipa_interconnect structure contains an icc_path pointer, plus an
average and peak bandwidth value.  Other than the interconnect name,
this matches the icc_bulk_data structure exactly.

Use the icc_bulk_data structure in place of the ipa_interconnect
structure, and add an initialization of its name field.  Then get
rid of the now unnecessary ipa_interconnect structure definition.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

In some cases, the IPA hardware needs to request the always-on
subsystem (AOSS) to coordinate with the IPA microcontroller to
retain IPA register values at power collapse.  This is done by
issuing a QMP request to the AOSS microcontroller.  A similar
request ondoes that request.

We must get and hold the "QMP" handle early, because we might get
back EPROBE_DEFER for that.  But the actual request should be sent
while we know the IPA clock is active, and when we know the
microcontroller is operational.

Fixes: 1aac309d

 ("net: ipa: use autosuspend")
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Finally, rename "ipa_clock.c" to be "ipa_power.c" and "ipa_clock.h"
to be "ipa_power.h".
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Rename a number of functions to clarify that there is no longer a
notion of an "IPA clock," but rather that the functions are more
generally related to IPA power management.

  ipa_clock_enable() -> ipa_power_enable()
  ipa_clock_disable() -> ipa_power_disable()
  ipa_clock_rate() -> ipa_core_clock_rate()
  ipa_clock_init() -> ipa_power_init()
  ipa_clock_exit() -> ipa_power_exit()

Rename the ipa_clock structure to be ipa_power.  Rename all
variables and fields using that structure type "power" rather
than "clock".

Rename the ipa_clock_data structure to be ipa_power_data, and more
broadly, just substitute "power" for "clock" in places that
previously represented things related to the "IPA clock".

Update comments throughout.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Use runtime power management autosuspend.

Up until this point, we only suspended the IPA hardware for system
suspend; now we'll suspend it aggressively using runtime power
management, setting the initial autosuspend delay to half a second
of inactivity.

Replace pm_runtime_put() calls with pm_runtime_put_autosuspend(),
call pm_runtime_mark_last_busy() before each of those.  In places
where we're shutting things down, or decrementing power references
for errors, use pm_runtime_put_noidle() instead.

Finally, remove ipa_runtime_idle(), so the ->runtime_suspend
callback will occur if idle.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

The only remaining user of the ipa_clock_{get,put}() interface is
ipa_isr_thread().  Replace calls to ipa_clock_get() there calling
pm_runtime_get_sync() instead.  And call pm_runtime_put() there
rather than ipa_clock_put().  Warn if we ever get an error.

With that, we can get rid of ipa_clock_get() and ipa_clock_put().
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Jakub Kicinski pointed out a race condition in ipa_start_xmit() in a
recently-accepted series of patches:
  https://lore.kernel.org/netdev/20210812195035.2816276-1-elder@linaro.org/
We are stopping the modem TX queue in that function if the power
state is not active.  We restart the TX queue again once hardware
resume is complete.

  TX path                       Power Management
  -------                       ----------------
  pm_runtime_get(); no power    Start resume
  Stop TX queue                      ...
  pm_runtime_put()              Resume complete
  return NETDEV_TX_BUSY         Start TX queue

  pm_runtime_get()
  Power present, transmit
  pm_runtime_put()              (auto-suspend)

The issue is that the power management (resume) activity and the
network transmit activity can occur concurrently, and there's a
chance the queue will be stopped *after* it has been started again.

  TX path                       Power Management
  -------                       ----------------
                                Resume underway
  pm_runtime_get(); no power         ...
                                Resume complete
                                Start TX queue
  Stop TX queue       <-- No more transmits after this
  pm_runtime_put()
  return NETDEV_TX_BUSY

We address this using a STARTED flag to indicate when the TX queue
has been started from the resume path, and a spinlock to make the
flag and queue updates happen atomically.

  TX path                       Power Management
  -------                       ----------------
                                Resume underway
  pm_runtime_get(); no power    Resume complete
                                start TX queue     \
  If STARTED flag is *not* set:                     > atomic
      Stop TX queue             set STARTED flag   /
  pm_runtime_put()
  return NETDEV_TX_BUSY

A second flag is used to address a different race that involves
another path requesting power.

  TX path            Other path              Power Management
  -------            ----------              ----------------
                     pm_runtime_get_sync()   Resume
                                             Start TX queue   \ atomic
                                             Set STARTED flag /
                     (do its thing)
                     pm_runtime_put()
                                             (auto-suspend)
  pm_runtime_get()                           Mark delayed resume
  STARTED *is* set, so
    do *not* stop TX queue  <-- Queue should be stopped here
  pm_runtime_put()
  return NETDEV_TX_BUSY                      Suspend done, resume
                                             Resume complete
  pm_runtime_get()
  Stop TX queue
    (STARTED is *not* set)                   Start TX queue   \ atomic
  pm_runtime_put()                           Set STARTED flag /
  return NETDEV_TX_BUSY

So a STOPPED flag is set in the transmit path when it has stopped
the TX queue, and this pair of operations is also protected by the
spinlock.  The resume path only restarts the TX queue if the STOPPED
flag is set.  This case isn't a major problem, but it avoids the
"non-trivial amount of useless work" done by the networking stack
when NETDEV_TX_BUSY is returned.

Fixes: 6b51f802

 ("net: ipa: ensure hardware has power in ipa_start_xmit()")
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Add a new flag that is set when the hardware is suspended due to a
system suspend operation, distingishing it from runtime suspend.
Use it in the SUSPEND IPA interrupt handler to determine whether to
trigger a system resume because of the event.  Define new suspend
and resume power management callback functions to set and clear the
new flag, respectively.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Move the call to enable the IPA interrupt as a wakeup interrupt into
ipa_power_setup(), disable it in ipa_power_teardown().
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Now that ipa_clock_get_additional() is a trivial wrapper around
pm_runtime_get_if_active(), just open-code it in its only caller
and delete the function.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

The runtime power management core code maintains a usage count.  This
count mirrors the IPA clock reference count, and there's no need to
maintain both.  So get rid of the IPA clock reference count and just
rely on the runtime PM usage count to determine when the hardware
should be suspended or resumed.

Use pm_runtime_get_if_active() in ipa_clock_get_additional().  We
care whether power is active, regardless of whether it's in use, so
pass true for its ign_usage_count argument.

The IPA clock mutex is just used to make enabling/disabling the
clock and updating the reference count occur atomically.  Without
the reference count, there's no need for the mutex, so get rid of
that too.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Use the runtime power management core to cause hardware suspend and
resume to occur.  Enable it in ipa_clock_init() (without autosuspend),
and disable it in ipa_clock_exit().

Use ipa_runtime_suspend() as the ->runtime_suspend power operation,
and arrange for it to be called by having ipa_clock_get() call
pm_runtime_get_sync() when the first clock reference is taken.
Similarly, use ipa_runtime_resume() as the ->runtime_resume power
operation, and pm_runtime_put() when the last IPA clock reference
is dropped.

Introduce ipa_runtime_idle() as the ->runtime_idle power operation,
and have it return a non-zero value; this way suspend will never
occur except when forced.

Use pm_runtime_force_suspend() and pm_runtime_force_resume() as the
system suspend and resume callbacks, and remove ipa_suspend() and
ipa_resume().

Store a pointer to the device structure passed to ipa_clock_init(),
so it can be used by ipa_clock_exit() to disable runtime power
management.

For now we preserve IPA clock reference counting.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Introduce ipa_runtime_suspend() and ipa_runtime_resume(), which
encapsulate the activities necessary for suspending and resuming
the IPA hardware.  Call these functions from ipa_clock_get() and
ipa_clock_put() when the first reference is taken or last one is
dropped.

When the very first clock reference is taken (for ipa_config()),
setup isn't complete yet, so (as before) only the core clock gets
enabled.

When the last clock reference is dropped (after ipa_deconfig()),
ipa_teardown() will have made the setup_complete flag false, so
there too, the core clock will be stopped without affecting GSI
or the endpoints.

Otherwise these new functions will perform the desired suspend and
resume actions once setup is complete.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Disable the IPA clock rather than dropping a reference to it in the
system suspend callback.  This forces the suspend to occur without
affecting existing references.

Similarly, enable the clock rather than taking a reference in
ipa_resume(), forcing a resume without changing the reference count.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

We currently assume no errors occur when enabling or disabling the
IPA core clock and interconnects.  And although this commit exposes
errors that could occur, we generally assume this won't happen in
practice.

This commit changes ipa_clock_get() and ipa_clock_put() so each
returns a value.  The values returned are meant to mimic what the
runtime power management functions return, so we can set up error
handling here before we make the switch.  Have ipa_clock_get()
increment the reference count even if it returns an error, to match
the behavior of pm_runtime_get().

More details follow.

When taking a reference in ipa_clock_get(), return 0 for the first
reference, 1 for subsequent references, or a negative error code if
an error occurs.  Note that if ipa_clock_get() returns an error, we
must not touch hardware; in some cases such errors now cause entire
blocks of code to be skipped.

When dropping a reference in ipa_clock_put(), we return 0 or an
error code.  The error would come from ipa_clock_disable(), which
now returns what ipa_interconnect_disable() returns (either 0 or a
negative error code).  For now, callers ignore the return value;
if an error occurs, a message will have already been logged, and
little more can actually be done to improve the situation.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

The ipa->flags field is only ever used in "ipa_clock.c", related to
suspend/resume activity.

Move the definition of the ipa_flag enumerated type to "ipa_clock.c".
And move the flags field from the ipa structure and to the ipa_clock
structure.  Rename the type and its values to include "power" or
"POWER" in the name.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Move ipa_suspend_handler() into "ipa_clock.c" from "ipa_main.c", to
group with the reset of the suspend/resume code.  This IPA interrupt
is triggered if an IPA RX endpoint is suspended but has a packet to
be delivered.

Introduce ipa_power_setup() and ipa_power_teardown() to add and
remove the handler for the IPA SUSPEND interrupt at the same place
as before, while allowing the handler to remain private.

The "power" naming convention will be adopted elsewhere in this
file as well (soon).
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Move ipa_suspend() and ipa_resume(), as well as the definition of
the ipa_pm_ops structure into "ipa_clock.c".  Make ipa_pm_ops public
and declare it as extern in "ipa_clock.h".

This is part of centralizing IPA power management functionality into
"ipa_clock.c" (the file will eventually get a name change).
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Rearrange messages reported when errors occur in the IPA clock code,
so that the specific interconnect is identified when an error occurs
enabling or disabling it, or the core clock is indicated when an
error occurs enabling it.

Have ipa_interconnect_disable() return zero or the negative error
value returned by the first interconnect that produced an error
when disabled.  For now, the callers ignore the returned value.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

When initializing the IPA core clock and interconnects, it's
possible we'll get an EPROBE_DEFER error.  This isn't really an
error, it's just means we need to be re-probed later.

Use dev_err_probe() to report the error rather than dev_err().
This avoids polluting the log with these "error" messages.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Currently we assume that the IPA hardware has exactly three
interconnects.  But that won't be guaranteed for all platforms,
so allow any number of interconnects to be specified in the
configuration data.

For each platform, define an array of interconnect data entries
(still associated with the IPA clock structure), and record the
number of entries initialized in that array.

Loop over all entries in this array when initializing, enabling,
disabling, or tearing down the set of interconnects.

With this change we no longer need the ipa_interconnect_id
enumerated type, so get rid of it.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Pass an the address of an IPA interconnect structure and its
configuration data to ipa_interconnect_init_one() and have that
function initialize all the structure's fields.  Change the function
to simply return an error code.

Introduce ipa_interconnect_exit_one() to encapsulate the cleanup of
an IPA interconnect structure.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Add the name to the configuration data for each interconnect.  Use
this information rather than a constant string during initialization.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Add fields in the ipa_interconnect structure to hold the average and
peak bandwidth values for the interconnect.  Pass the configuring
data for interconnects to ipa_interconnect_init() so these values
can be recorded, and use them when enabling the interconnects.

There's no longer any need to keep a copy of the interconnect data
after initialization.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Rather than having separate pointers for the memory, imem, and
config interconnect paths, maintain an array of ipa_interconnect
structures each of which contains a pointer to a path.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

If disabling interconnects fails there's not a lot we can do.  The
only two callers of ipa_interconnect_disable() ignore the return
value, so just give the function a void return type.

Print an error message if disabling any of the interconnects is not
successful.  Return (and print) only the first error seen.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Use "bandwidth" rather than "rate" in describing the average and
peak values to use for IPA interconnects.  They should have been
named that way to begin with.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

When the core clock rate and interconnect bandwidth specifications
were moved into configuration data, a copy/paste bug was introduced,
causing the memory interconnect bandwidth to be set three times
rather than enabling the three different interconnects.

Fix this bug.

Fixes: 91d02f95

 ("net: ipa: use config data for clocking")
Signed-off-by: Alex Elder <elder@linaro.org>
Reviewed-by: Georgi Djakov <georgi.djakov@linaro.org>
Link: https://lore.kernel.org/r/20201222151613.5730-1-elder@linaro.org

Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Stop assuming a fixed IPA core clock rate and interconnect
bandwidths.  Use the configuration data defined for these
things instead.  Get rid of the previously-used constants.
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Define a new type of configuration data, used to initialize the
IPA core clock and interconnects.  This is the first of three
patches, and defines the data types and interface but doesn't
yet use them.

Switch the return value if there is no matching configuration data
to ENODEV instead of ENOTSUPP (to avoid using the nonstandard errno).
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Currently, when (before) the last IPA clock reference is dropped,
all endpoints are suspended.  And whenever the first IPA clock
reference is taken, all endpoints are resumed (or started).

In most cases there's no need to start endpoints when the clock
starts.  So move the calls to ipa_endpoint_suspend() and
ipa_endpoint_resume() out of ipa_clock_put() and ipa_clock_get(),
respectiely.  Instead, only suspend endpoints when handling a system
suspend, and only resume endpoints when handling a system resume.
Signed-off-by: Alex Elder <elder@linaro.org>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

Take advantage of the checking provided by refcount_t, rather than
using a plain atomic to represent the IPA clock reference count.

Note that we need to *set* the value to 1 in ipa_clock_get() rather
than incrementing it from 0 (because doing that is considered an
error for a refcount_t).
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>

This commit affects comments (and in one case, whitespace) only.

Throughout the IPA code, return statements are documented using
"@Return:", whereas they should use "Return:" instead.  Fix these
mistakes.

In function definitions, some parameters are missing their comment
to describe them.  And in structure definitions, some fields are
missing their comment to describe them.  Add these missing
descriptions.

Some arguments changed name and type along the way, but their
descriptions were not updated (an endpoint pointer is now used in
many places that previously used an endpoint ID).  Fix these
incorrect parameter descriptions.

In the description for the ipa_clock structure, one field had a
semicolon instead of a colon in its description.  Fix this.

Add a missing function description for ipa_gsi_endpoint_data_empty().

All of these issues were identified when building with "W=1".
Signed-off-by: Alex Elder <elder@linaro.org>
Signed-off-by: David S. Miller <davem@davemloft.net>