| Age | Commit message (Collapse) | Author | Files | Lines |
|---|
|
It's possible for interrupts to get significantly delayed to the point
that callers of intel_scu_ipc_dev_command() and friends can call the
function once, hit a timeout, and call it again while the interrupt
still hasn't been processed. This driver will get seriously confused if
the interrupt is finally processed after the second IPC has been sent
with ipc_command(). It won't know which IPC has been completed. This
could be quite disastrous if calling code assumes something has happened
upon return from intel_scu_ipc_dev_simple_command() when it actually
hasn't.
Let's avoid this scenario by simply returning -EBUSY in this case.
Hopefully higher layers will know to back off or fail gracefully when
this happens. It's all highly unlikely anyway, but it's better to be
correct here as we have no way to know which IPC the status register is
telling us about if we send a second IPC while the previous IPC is still
processing.
Cc: Prashant Malani <[email protected]>
Cc: Kuppuswamy Sathyanarayanan <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Fixes: ed12f295bfd5 ("ipc: Added support for IPC interrupt mode")
Signed-off-by: Stephen Boyd <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Reviewed-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
|
|
intel_scu_ipc_dev_simple_command()
Andy discovered this bug during patch review. The 'scu' argument to this
function shouldn't be overridden by the function itself. It doesn't make
any sense. Looking at the commit history, we see that commit
f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
removed the setting of the scu to ipcdev in other functions, but not
this one. That was an oversight. Remove this line so that we stop
overriding the scu instance that is used by this function.
Reported-by: Andy Shevchenko <[email protected]>
Closes: https://lore.kernel.org/r/[email protected]
Cc: Prashant Malani <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Fixes: f57fa18583f5 ("platform/x86: intel_scu_ipc: Introduce new SCU IPC API")
Signed-off-by: Stephen Boyd <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Reviewed-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
|
|
ipc_wait_for_interrupt()
It's possible for the completion in ipc_wait_for_interrupt() to timeout,
simply because the interrupt was delayed in being processed. A timeout
in itself is not an error. This driver should check the status register
upon a timeout to ensure that scheduling or interrupt processing delays
don't affect the outcome of the IPC return value.
CPU0 SCU
---- ---
ipc_wait_for_interrupt()
wait_for_completion_timeout(&scu->cmd_complete)
[TIMEOUT] status[IPC_STATUS_BUSY]=0
Fix this problem by reading the status bit in all cases, regardless of
the timeout. If the completion times out, we'll assume the problem was
that the IPC_STATUS_BUSY bit was still set, but if the status bit is
cleared in the meantime we know that we hit some scheduling delay and we
should just check the error bit.
Cc: Prashant Malani <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Fixes: ed12f295bfd5 ("ipc: Added support for IPC interrupt mode")
Signed-off-by: Stephen Boyd <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Reviewed-by: Hans de Goede <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
|
|
It's possible for the polling loop in busy_loop() to get scheduled away
for a long time.
status = ipc_read_status(scu); // status = IPC_STATUS_BUSY
<long time scheduled away>
if (!(status & IPC_STATUS_BUSY))
If this happens, then the status bit could change while the task is
scheduled away and this function would never read the status again after
timing out. Instead, the function will return -ETIMEDOUT when it's
possible that scheduling didn't work out and the status bit was cleared.
Bit polling code should always check the bit being polled one more time
after the timeout in case this happens.
Fix this by reading the status once more after the while loop breaks.
The readl_poll_timeout() macro implements all of this, and it is
shorter, so use that macro here to consolidate code and fix this.
There were some concerns with using readl_poll_timeout() because it uses
timekeeping, and timekeeping isn't running early on or during the late
stages of system suspend or early stages of system resume, but an audit
of the code concluded that this code isn't called during those times so
it is safe to use the macro.
Cc: Prashant Malani <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Reviewed-by: Kuppuswamy Sathyanarayanan <[email protected]>
Fixes: e7b7ab3847c9 ("platform/x86: intel_scu_ipc: Sleeping is fine when polling")
Signed-off-by: Stephen Boyd <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Reviewed-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Hans de Goede <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
|
|
There is no need to manually set the owner of a struct class, as the
registering function does it automatically, so remove all of the
explicit settings from various drivers that did so as it is unneeded.
This allows us to remove this pointer entirely from this structure going
forward.
Cc: "Rafael J. Wysocki" <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Greg Kroah-Hartman <[email protected]>
|
|
__intel_scu_ipc_register()
In some error paths before device_register(), the names allocated
by dev_set_name() are not freed. Move dev_set_name() front to
device_register(), so the name can be freed while calling
put_device().
Fixes: 54b34aa0a729 ("platform/x86: intel_scu_ipc: Split out SCU IPC functionality from the SCU driver")
Signed-off-by: Yang Yingliang <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Reviewed-by: Hans de Goede <[email protected]>
Signed-off-by: Hans de Goede <[email protected]>
|
|
The comment decribing the IPC timeout hadn't been updated when the
actual timeout was changed from 3 to 5 seconds in
commit a7d53dbbc70a ("platform/x86: intel_scu_ipc: Increase virtual
timeout from 3 to 5 seconds") .
Since the value is anyway updated to 10s now, take this opportunity to
update the value in the comment too.
Signed-off-by: Prashant Malani <[email protected]>
Cc: Benson Leung <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
Commit a7d53dbbc70a ("platform/x86: intel_scu_ipc: Increase virtual
timeout from 3 to 5 seconds") states that the recommended timeout range
is 5-10 seconds. Adjust the timeout value to the higher of those i.e 10
seconds, to account for situations where the 5 seconds is insufficient
for disconnect command success.
Signed-off-by: Prashant Malani <[email protected]>
Cc: Benson Leung <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
The macro IPC_TIMEOUT is already in jiffies (it is also used like that
elsewhere in the file when calling wait_for_completion_timeout()). Don’t
convert it using helper functions for the purposes of calculating the
busy loop expiry time.
Fixes: e7b7ab3847c9 (“platform/x86: intel_scu_ipc: Sleeping is fine when polling”)
Signed-off-by: Prashant Malani <[email protected]>
Cc: Benson Leung <[email protected]>
Reviewed-by: Mika Westerberg <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
The kernel doc validator complains:
.../ipc.c:478: warning: expecting prototype for intel_scu_ipc_command_with_size(). Prototype was for intel_scu_ipc_dev_command_with_size() instead
Fix the prototype name in the kernel documentation.
Reported-by: kernel test robot <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
Increasing the virtual timeout time to account for scenarios
that may require more time, like DisplayPort Multi-Stream Transport
(DP MST), where the disconnect time can be extended longer than
usual.
The recommended timeout range is 5-10 seconds, of which
we will take the lower bound.
Signed-off-by: Casey Bowman <[email protected]>
Acked-by: Heikki Krogerus <[email protected]>
Acked-by: Mika Westerberg <[email protected]>
Acked-by: Andy Shevchenko <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Hans de Goede <[email protected]>
|
|
Drivers such as intel_pmc_ipc.c can be unloaded as well so in order to
support those in this driver add a new function that can be called to
unregister the SCU IPC when it is not needed anymore.
We also add a managed version of the intel_scu_ipc_register() that takes
care of calling intel_scu_ipc_unregister() automatically when the driver
is unbound.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
|
|
The current SCU IPC API has been operating on a single instance and
there has been no way to pin the providing module in place when the SCU
IPC is in use.
This implements a new API that takes the SCU IPC instance as first
parameter (NULL means the single instance is being used). The SCU IPC
instance can be retrieved by calling new function intel_scu_ipc_dev_get()
that take care of pinning the providing module in place as long as
intel_scu_ipc_dev_put() is not called.
The old API is updated to call the new API and is is left there in the
legacy API header to support the existing users that cannot be converted
easily.
Subsequent patches will convert most of the users over to the new API.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
|
|
Currently we only log an error if the command times out which makes it
hard to figure out the failing command. This changes the driver to log
command and subcommand with the error code which should make debugging
easier. This also allows us to simplify the callers as they don't need
to log these errors themselves.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
|
|
The SCU IPC functionality is usable outside of Intel MID devices. For
example modern Intel CPUs include the same thing but now it is called
PMC (Power Management Controller) instead of SCU. To make the IPC
available for those split the driver into core part (intel_scu_ipc.c)
and the SCU PCI driver part (intel_scu_pcidrv.c) which then calls the
former before it goes and creates rest of the SCU devices. The SCU IPC
will also register a new class that gets assigned to the device that is
created under the parent PCI device.
We also split the Kconfig symbols so that INTEL_SCU_IPC enables the SCU
IPC library and INTEL_SCU_PCI the SCU driver and convert the users
accordingly. While there remove default y from the INTEL_SCU_PCI symbol
as it is already selected by X86_INTEL_MID.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Lee Jones <[email protected]>
|
|
Format kernel-doc comments of the exported functions to follow the
typical format that does not have tab indentation. Also capitalize
parameter descriptions and add a missing period.
No functional changes intended.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
There is no user for this function so we can drop it from the driver.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
There are no users for these so we can remove them.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
These macros are not used anywhere in the driver so drop them.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
There is no reason why the driver would need to block other threads from
running the CPU while it is waiting for the SCU IPC to complete its
work. For this reason switch the driver to use usleep_range() instead
with a bit more relaxed polling loop.
Also add constant for the timeout and use the same value for both
polling and interrupt modes.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
There are no existing users for this functionality so drop it from the
driver completely. This also means we don't need to keep the struct
intel_scu_ipc_pdata_t around anymore so remove that as well.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
Moorestown support was removed years ago with by the commit 1a8359e411eb
("x86/mid: Remove Intel Moorestown"). Lincroft is the CPU side chip of
Moorestown and not supported anymore so remove the code from the driver.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
This makes the code more readable. These are taken from intel_pmc_ipc.c
which implements the same thing.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
Currently the driver has disabled interrupt support for Tangier but
actually interrupt works just fine if the command is not written twice
in a row. Also we need to ack the interrupt in the handler.
Signed-off-by: Mika Westerberg <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Cc: [email protected]
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
Reduce size of duplicated comments by switching to use SPDX identifier.
No functional change.
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
Sort headers alphabetically for better maintenance.
No functional change.
While here, remove unneeded linux/init.h inclusion.
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
intel_scu_ipc_i2c_cntrl
intel_scu_ipc_i2c_cntrl() calls mutex_lock(), which indicates
this function is not called in atomic context.
Despite never getting called from atomic context,
intel_scu_ipc_i2c_cntrl() calls mdelay to busily wait.
This is not necessary and can be replaced with usleep_range to
avoid busy waiting.
This is found by a static analysis tool named DCNS written by myself.
And I also manually check it.
Signed-off-by: Jia-Ju Bai <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
Make these const as they are only used during a copy operation.
Done using Coccinelle.
Signed-off-by: Bhumika Goyal <[email protected]>
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
A new call to SCU intel_scu_ipc_raw_command() writes SPTR and DPTR
registers before sending a command.
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
For better maintainability and readability introduce a macro
for device ID table.
No functional change intended.
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
The driver is bound to the devices based on their PCI IDs.
There is no need to do an additional check.
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
Device pointer is used as a flag that everything is prepared.
Nevertheless the assignment happened quite before and there is a window
when a caller can get weird results or even crashes since not all fields
are initialized yet.
Rearrange initialization sequence in ->probe() to prepare everything
before use.
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
Fail ->probe() if there is no platform data supplied.
Signed-off-by: Andy Shevchenko <[email protected]>
|
|
The Kconfig currently controlling compilation of this code is:
drivers/platform/x86/Kconfig:config INTEL_SCU_IPC
drivers/platform/x86/Kconfig: bool "Intel SCU IPC Support"
...meaning that it currently is not being built as a module by anyone.
Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.
We explicitly disallow a driver unbind, since that doesn't have a
sensible use case anyway, and it allows us to drop the ".remove"
code for non-modular drivers.
Since module_pci_driver() uses the same init level priority as
builtin_pci_driver() the init ordering remains unchanged with
this commit.
Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.
We don't replace module.h with init.h since the file already has that.
We also delete the MODULE_LICENSE tag etc. since all that information
is already contained at the top of the file in the comments.
Cc: [email protected]
Signed-off-by: Paul Gortmaker <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
Protect the dev member assignment in ->remove() since user may potentially call
unbind from a sysfs even if the driver is built-in. The latter might be racy
with ongoing SCU communication.
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
Eliminate some boilerplate code by using module_pci_driver() instead of
init/exit, moving the salient bits from init into probe.
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
Switch the code to use struct device * instead of struct pci_dev * since there
is no reason to access PCI related features in the driver.
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
As much as possible propagate a pointer to struct intel_scu_ipc_dev.
There is no functional change.
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
The error handling is broken right now since it leaves resources unfreed.
Convert the code to use managed resources to fix the error handling.
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
'{ }' and memset will both reset the cbuf buffer.
Only once is enough and this can be done outside fo the mutex.
Signed-off-by: Christophe JAILLET <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
Read the resources from PCI BAR0 instead of using hardcoded values.
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
This is small performance optimization of the busy_loop().
While here, use BIT() macro instead of plain integers when check the status.
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
While here, do couple of amendments:
- move platform variable to the function where it's used
- define intel_scu_ipc_check_status() static
Signed-off-by: Andy Shevchenko <[email protected]>
Signed-off-by: Darren Hart <[email protected]>
|
|
We should prefer `struct pci_device_id` over `DEFINE_PCI_DEVICE_TABLE` to
meet kernel coding style guidelines. This issue was reported by checkpatch.
A simplified version of the semantic patch that makes this change is as
follows (http://coccinelle.lip6.fr/):
// <smpl>
@@
identifier i;
declarer name DEFINE_PCI_DEVICE_TABLE;
initializer z;
@@
- DEFINE_PCI_DEVICE_TABLE(i)
+ const struct pci_device_id i[]
= z;
// </smpl>
[bhelgaas: add semantic patch]
Signed-off-by: Benoit Taine <[email protected]>
Signed-off-by: Bjorn Helgaas <[email protected]>
|
|
For readability matters, this patch replaces the hardcoded pci ids by
human readable macros.
Signed-off-by: David Cohen <[email protected]>
Signed-off-by: Matthew Garrett <[email protected]>
|
|
This patch removes the unnecessary enum for platform type to handle the
array of pdatas. We can set pdata directly to pci_device_id struct
instead.
Signed-off-by: David Cohen <[email protected]>
Signed-off-by: Matthew Garrett <[email protected]>
|
|
This patch adds support for ipc command interrupt mode.
Also added platform data option to select 'irq_mode'
irq_mode = 1: configure the driver to receive IOC interrupt
for each successful ipc_command.
irq_mode = 0: makes driver use polling method to
track the command completion status.
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: David Cohen <[email protected]>
Signed-off-by: Matthew Garrett <[email protected]>
|
|
Handle error conditions in intel_scu_ipc_command() and
pwr_reg_rdwr().
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Signed-off-by: David Cohen <[email protected]>
Signed-off-by: Matthew Garrett <[email protected]>
|
|
Enabled ipc support for penwell, clovertrail & tangier platforms.
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Cc: David Cohen <[email protected]>
Signed-off-by: Matthew Garrett <[email protected]>
|
|
Since the same ipc driver can be used by many platforms, using
macros for defining ipc_base and i2c_base addresses is not
a scalable approach. So added a platform data structure to pass
this information.
Signed-off-by: Kuppuswamy Sathyanarayanan <[email protected]>
Acked-by: Alan Cox <[email protected]>
Cc: David Cohen <[email protected]>
Signed-off-by: Matthew Garrett <[email protected]>
|