Thanks for the review Bjorn !
On 9/30/2024 9:16 AM, Bjorn Andersson wrote:
On Fri, Sep 27, 2024 at 12:01:08PM GMT, Mukesh Kumar Savaliya wrote:
Add support to share I2C SE by two Subsystems in a mutually exclusive way.
Use "qcom,shared-se" flag in a particular i2c instance node if the usecase
requires i2c controller to be shared.
Please start your commit message by describing the problem your patch
is solving.
Done
Sharing of SE(Serial engine) is possible only for GSI mode as each
subsystem(SS) can queue transfers over its own GPII Channel. For non GSI
mode, we should force disable this feature even if set by user from DT by
mistake.
I2C driver just need to mark first_msg and last_msg flag to help indicate
GPI driver to take lock and unlock TRE there by protecting from concurrent
access from other EE or Subsystem.
gpi_create_i2c_tre() function at gpi.c will take care of adding Lock and
Unlock TRE for the respective transfer operations.
Since the GPIOs are also shared between two SS, do not unconfigure them
during runtime suspend. This will allow other SS to continue to transfer
the data without any disturbance over the IO lines.
This last paragraph describes patch 3, right?
Yes, i think i should i keep it in patch 3.
Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@xxxxxxxxxxx>
---
drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 212336f724a6..479fa8e1c33f 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
// Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
#include <linux/acpi.h>
#include <linux/clk.h>
@@ -602,6 +603,7 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
peripheral.clk_div = itr->clk_div;
peripheral.set_config = 1;
peripheral.multi_msg = false;
+ peripheral.shared_se = gi2c->se.shared_geni_se;
for (i = 0; i < num; i++) {
gi2c->cur = &msgs[i];
@@ -612,6 +614,8 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
if (i < num - 1)
peripheral.stretch = 1;
+ peripheral.first_msg = (i == 0);
+ peripheral.last_msg = (i == num - 1);
There are multiple error paths in this loop, which would result in us
never issuing the unlock TRE - effectively blocking other subsystems
from accessing the serial engine until we perform our next access
(assuming that APSS issuing a lock TRE when APSS already has the channel
locked isn't a problem?)
peripheral.addr = msgs[i].addr;
ret = geni_i2c_gpi(gi2c, &msgs[i], &config,
@@ -631,8 +635,11 @@ static int geni_i2c_gpi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], i
dma_async_issue_pending(gi2c->tx_c);
time_left = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
- if (!time_left)
+ if (!time_left) {
+ dev_dbg(gi2c->se.dev, "I2C timeout gpi flags:%d addr:0x%x\n",
+ gi2c->cur->flags, gi2c->cur->addr);
This looks useful, but unrelated to this patch.
Sure, Removed it.
gi2c->err = -ETIMEDOUT;
+ }
if (gi2c->err) {
ret = gi2c->err;
@@ -800,6 +807,11 @@ static int geni_i2c_probe(struct platform_device *pdev)
gi2c->clk_freq_out = KHZ(100);
}
+ if (of_property_read_bool(pdev->dev.of_node, "qcom,shared-se")) {
+ gi2c->se.shared_geni_se = true;
gi2c->se.shared_geni_se = of_property_read_bool(dev->of_node, "qcom,shared-se");
+ dev_dbg(&pdev->dev, "I2C is shared between subsystems\n");
+ }
+
if (has_acpi_companion(dev))
ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
@@ -870,8 +882,10 @@ static int geni_i2c_probe(struct platform_device *pdev)
else
fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
- if (fifo_disable) {
- /* FIFO is disabled, so we can only use GPI DMA */
+ if (fifo_disable || gi2c->se.shared_geni_se) {
+ /* FIFO is disabled, so we can only use GPI DMA.
+ * SE can be shared in GSI mode between subsystems, each SS owns a GPII.
+ **/
I think you're trying to document why we're entering the "GPI-only"
branch. The addition you made was that if the user has requested
"shared-se", then it's GPI-only.
yes, that's right.
But I'm not able to wrap my head around your addition here. Why does it
matter that each subsystem own a GPII? Is that a reason for choosing
GPI-only mode?
Not sure i got your question here.
The feature flag true means it should be in GPI mode.
gi2c->gpi_mode = true;
ret = setup_gpi_dma(gi2c);
if (ret) {
@@ -883,6 +897,9 @@ static int geni_i2c_probe(struct platform_device *pdev)
dev_dbg(dev, "Using GPI DMA mode for I2C\n");
} else {
gi2c->gpi_mode = false;
+
+ /* Force disable shared SE case for non GSI mode */
GSI or GPI mode?
Changed to GPI.
+ gi2c->se.shared_geni_se = false;
If shared_geni_se was true prior to this assignment, wouldn't we have
entered the if (fifo_disable ...) branch?
In that case also we should enter the condition of executing in GPI mode.
tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
/* I2C Master Hub Serial Elements doesn't have the HW_PARAM_0 register */
@@ -964,7 +981,6 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
if (ret) {
enable_irq(gi2c->irq);
return ret;
-
Please avoid such unrelated cleanups.
Sure
Regards,
Bjorn
} else {
gi2c->suspended = 1;
}
--
2.25.1
