| Message ID | 20240927063108.2773304-1-quic_msavaliy@quicinc.com |
|---|---|
| Headers | show |
| Series | Enable shared SE support over I2C | expand |
On Fri, Sep 27, 2024 at 12:01:07PM GMT, Mukesh Kumar Savaliya wrote: > Currently the driver provides a function called geni_serial_resources_off() > to turn off resources like clocks and pinctrl. > > For shared SE between two SS, we don't need to keep pinctrl to sleep state > as other SS may be actively transferring data over SE. "don't need to" sounds like an optimization. Is this really the case? The comment in the code below seems to indicate no. As with the other commit message, expand your abbreviations. > Hence,bypass keeping > pinctrl to sleep state conditionally using shared_geni_se flag. > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > --- > drivers/soc/qcom/qcom-geni-se.c | 14 ++++++++++---- > include/linux/soc/qcom/geni-se.h | 3 +++ > 2 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > index 2e8f24d5da80..89cf18699336 100644 > --- a/drivers/soc/qcom/qcom-geni-se.c > +++ b/drivers/soc/qcom/qcom-geni-se.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. > > /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */ > #define __DISABLE_TRACE_MMIO__ > @@ -503,10 +504,15 @@ int geni_se_resources_off(struct geni_se *se) > > if (has_acpi_companion(se->dev)) > return 0; > - > - ret = pinctrl_pm_select_sleep_state(se->dev); > - if (ret) > - return ret; > + /* Keep pinctrl to sleep state only for regular usecase. > + * Do not sleep pinctrl for shared SE because other SS(subsystems) > + * may continueto perform transfer. > + */ > + if (se->shared_geni_se == false) { > + ret = pinctrl_pm_select_sleep_state(se->dev); I'm a bit rusty on the pinctrl API, but wouldn't you achieve the same result by just not specifying a "sleep" pinctrl state? > + if (ret) > + return ret; > + } > > geni_se_clks_off(se); > return 0; > diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h > index c3bca9c0bf2c..359041c64ad8 100644 > --- a/include/linux/soc/qcom/geni-se.h > +++ b/include/linux/soc/qcom/geni-se.h > @@ -1,6 +1,7 @@ > /* 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. > */ > > #ifndef _LINUX_QCOM_GENI_SE > @@ -61,6 +62,7 @@ struct geni_icc_path { > * @num_clk_levels: Number of valid clock levels in clk_perf_tbl > * @clk_perf_tbl: Table of clock frequency input to serial engine clock > * @icc_paths: Array of ICC paths for SE > + * @shared_geni_se: Tells if SE is used by two SS in shared environment. Please avoid the abbreviations. Be succinct, e.g. does it matter that it's two SS - what if it's 3? "Tell" is not the correct verb here, struct members don't speak. Regards, Bjorn > */ > struct geni_se { > void __iomem *base; > @@ -70,6 +72,7 @@ struct geni_se { > unsigned int num_clk_levels; > unsigned long *clk_perf_tbl; > struct geni_icc_path icc_paths[3]; > + bool shared_geni_se; > }; > > /* Common SE registers */ > -- > 2.25.1 >
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. > 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? > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > --- > 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. > 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. 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? > 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? > + 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? > 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. Regards, Bjorn > } else { > gi2c->suspended = 1; > } > -- > 2.25.1 >
On Sun, Sep 29, 2024 at 10:46:37PM -0500, Bjorn Andersson wrote: > > @@ -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?) > Hi Bjorn, I saw the words "error paths" and "unlock" and I thought there was maybe something we could do here with static analysis. But I don't know what TRE or APSS mean. The one thing I do see is that this uses "one err" style error handling where there is one err label and it calls dmaengine_terminate_sync(gi2c->rx_c) regardless of whether or not geni_i2c_gpi() was called or failed/succeeded. regards, dan carpenter
On Mon, Sep 30, 2024 at 11:21:23AM GMT, Dan Carpenter wrote: > On Sun, Sep 29, 2024 at 10:46:37PM -0500, Bjorn Andersson wrote: > > > @@ -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?) > > > > Hi Bjorn, > > I saw the words "error paths" and "unlock" and I thought there was maybe > something we could do here with static analysis. Appreciate you picking up on those topics :) > But I don't know what TRE or APSS mean. > The "APSS" is "the application processor sub system", which is where we typically find Linux running. TRE is, if I understand correctly, a request on the DMA engine queue. > The one thing I do see is that this uses "one err" style error handling where > there is one err label and it calls dmaengine_terminate_sync(gi2c->rx_c) > regardless of whether or not geni_i2c_gpi() was called or failed/succeeded. > The scheme presented in this series injects a "LOCK" request before the DMA request marked first_msg, and an "UNLOCK" after the ones marked last_msg. This is needed because the controller is also concurrently by some DSP (or similar), so the LOCK/UNLOCK scheme forms mutual exclusion of the operations between the Linux and DSP systems. I'm not sure if it's possible to tie the unlock operation to dmaengine_terminate_sync() in some way. Giving this some more thought, it feels like the current scheme serves the purpose of providing mutual exclusion both for the controller and to some degree for the device. But I'd like to understand why we can't inject the lock/unlock implicitly for each transfer... Regards, Bjorn > regards, > dan carpenter >
On 27-09-24, 12:01, Mukesh Kumar Savaliya wrote: > GSI DMA provides specific TREs namely Lock and Unlock TRE, which > provides mutual exclusive access to SE from any of the subsystem > (E.g. Apps, TZ, ADSP etc). Lock prevents other subsystems from > concurrently performing DMA transfers and avoids disturbance to > data path. Basically for shared SE usecase, lock the SE for > particular subsystem, complete the transfer, unlock the SE. it is dmaengine: xxx so please update that > > Apply Lock TRE for the first transfer of shared SE and Apply Unlock > TRE for the last transfer. > > Also change MAX_TRE macro to 5 from 3 because of the two additional TREs. > > TRE = Transfer Ring Element, refers to the queued descriptor. > SE = Serial Engine > SS = Subsystems (Apps processor, TZ, ADSP, Modem) > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > --- > drivers/dma/qcom/gpi.c | 37 +++++++++++++++++++++++++++++++- > include/linux/dma/qcom-gpi-dma.h | 6 ++++++ > 2 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c > index 52a7c8f2498f..120d91234442 100644 > --- a/drivers/dma/qcom/gpi.c > +++ b/drivers/dma/qcom/gpi.c > @@ -2,6 +2,7 @@ > /* > * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved. > * Copyright (c) 2020, Linaro Limited > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > */ > > #include <dt-bindings/dma/qcom-gpi.h> > @@ -65,6 +66,14 @@ > /* DMA TRE */ > #define TRE_DMA_LEN GENMASK(23, 0) > > +/* Lock TRE */ > +#define TRE_I2C_LOCK BIT(0) > +#define TRE_MINOR_TYPE GENMASK(19, 16) > +#define TRE_MAJOR_TYPE GENMASK(23, 20) > + > +/* Unlock TRE */ > +#define TRE_I2C_UNLOCK BIT(8) > + > /* Register offsets from gpi-top */ > #define GPII_n_CH_k_CNTXT_0_OFFS(n, k) (0x20000 + (0x4000 * (n)) + (0x80 * (k))) > #define GPII_n_CH_k_CNTXT_0_EL_SIZE GENMASK(31, 24) > @@ -516,7 +525,7 @@ struct gpii { > bool ieob_set; > }; > > -#define MAX_TRE 3 > +#define MAX_TRE 5 > > struct gpi_desc { > struct virt_dma_desc vd; > @@ -1637,6 +1646,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc, > struct gpi_tre *tre; > unsigned int i; > > + /* create lock tre for first tranfser */ > + if (i2c->shared_se && i2c->first_msg) { > + tre = &desc->tre[tre_idx]; > + tre_idx++; > + > + tre->dword[0] = 0; > + tre->dword[1] = 0; > + tre->dword[2] = 0; > + tre->dword[3] = u32_encode_bits(1, TRE_I2C_LOCK); > + tre->dword[3] |= u32_encode_bits(0, TRE_MINOR_TYPE); > + tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE); > + } > + > /* first create config tre if applicable */ > if (i2c->set_config) { > tre = &desc->tre[tre_idx]; > @@ -1695,6 +1717,19 @@ static int gpi_create_i2c_tre(struct gchan *chan, struct gpi_desc *desc, > tre->dword[3] |= u32_encode_bits(1, TRE_FLAGS_IEOT); > } > > + /* Unlock tre for last transfer */ > + if (i2c->shared_se && i2c->last_msg && i2c->op != I2C_READ) { > + tre = &desc->tre[tre_idx]; > + tre_idx++; > + > + tre->dword[0] = 0; > + tre->dword[1] = 0; > + tre->dword[2] = 0; > + tre->dword[3] = u32_encode_bits(1, TRE_I2C_UNLOCK); > + tre->dword[3] |= u32_encode_bits(1, TRE_MINOR_TYPE); > + tre->dword[3] |= u32_encode_bits(3, TRE_MAJOR_TYPE); > + } > + > for (i = 0; i < tre_idx; i++) > dev_dbg(dev, "TRE:%d %x:%x:%x:%x\n", i, desc->tre[i].dword[0], > desc->tre[i].dword[1], desc->tre[i].dword[2], desc->tre[i].dword[3]); > diff --git a/include/linux/dma/qcom-gpi-dma.h b/include/linux/dma/qcom-gpi-dma.h > index 6680dd1a43c6..8589c711afae 100644 > --- a/include/linux/dma/qcom-gpi-dma.h > +++ b/include/linux/dma/qcom-gpi-dma.h > @@ -65,6 +65,9 @@ enum i2c_op { > * @rx_len: receive length for buffer > * @op: i2c cmd > * @muli-msg: is part of multi i2c r-w msgs > + * @shared_se: bus is shared between subsystems > + * @bool first_msg: use it for tracking multimessage xfer > + * @bool last_msg: use it for tracking multimessage xfer > */ > struct gpi_i2c_config { > u8 set_config; > @@ -78,6 +81,9 @@ struct gpi_i2c_config { > u32 rx_len; > enum i2c_op op; > bool multi_msg; > + bool shared_se; > + bool first_msg; > + bool last_msg; Looking at the usage in following patches, why cant this be handled internally as part of prep call? > }; > > #endif /* QCOM_GPI_DMA_H */ > -- > 2.25.1
On 27/09/2024 07:31, Mukesh Kumar Savaliya wrote:
> Add support to share I2C SE by two Subsystems in a mutually exclusive way.
As I read this the question jumps out "what is a subsystem" - in Linux
speak subsystem is say a bus or a memory management method but, here
what you really mean if I've understood the intent of this series is to
share the serial engine between two different bus-masters or perhaps a
better description is "system agent".
Please make that delination clear - its not two Linux subsystems but two
different Qcom SoC bus masters right ?
For example the APSS - Application Specific Sub Subsystem - where Linux
runs and say cDSP - the compute DSP on qcom SoCs.
I'd rename this patch to make that clear - because "between two
subsystems" if you aren't intimately versed in qcom's architecture
suggests that a Linux i2c and spi driver are somehow muxing pins ..
Really this is a type of AMP - asymmetric multi processing.
"i2c: i2c-qcom-geni: Enable i2c controller sharing between two different
bus masters"
And I'd mention in the commit log specific examples - APSS yes we get
but what is the other system agent in your use-case ?
A DSP ? Some other processor in the SoC ?
Anyway highlight one use-case for this AMP case, please.
---
bod
On 27.09.2024 8:31 AM, Mukesh Kumar Savaliya wrote: > GSI DMA provides specific TREs namely Lock and Unlock TRE, which > provides mutual exclusive access to SE from any of the subsystem > (E.g. Apps, TZ, ADSP etc). Lock prevents other subsystems from > concurrently performing DMA transfers and avoids disturbance to > data path. Basically for shared SE usecase, lock the SE for > particular subsystem, complete the transfer, unlock the SE. > > Apply Lock TRE for the first transfer of shared SE and Apply Unlock > TRE for the last transfer. > > Also change MAX_TRE macro to 5 from 3 because of the two additional TREs. > > TRE = Transfer Ring Element, refers to the queued descriptor. > SE = Serial Engine > SS = Subsystems (Apps processor, TZ, ADSP, Modem) > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > --- > drivers/dma/qcom/gpi.c | 37 +++++++++++++++++++++++++++++++- > include/linux/dma/qcom-gpi-dma.h | 6 ++++++ > 2 files changed, 42 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma/qcom/gpi.c b/drivers/dma/qcom/gpi.c > index 52a7c8f2498f..120d91234442 100644 > --- a/drivers/dma/qcom/gpi.c > +++ b/drivers/dma/qcom/gpi.c > @@ -2,6 +2,7 @@ > /* > * Copyright (c) 2017-2020, The Linux Foundation. All rights reserved. > * Copyright (c) 2020, Linaro Limited > + * Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved. > */ > > #include <dt-bindings/dma/qcom-gpi.h> > @@ -65,6 +66,14 @@ > /* DMA TRE */ > #define TRE_DMA_LEN GENMASK(23, 0) > > +/* Lock TRE */ > +#define TRE_I2C_LOCK BIT(0) > +#define TRE_MINOR_TYPE GENMASK(19, 16) > +#define TRE_MAJOR_TYPE GENMASK(23, 20) Is this solely I2C specific? Konrad
On 27.09.2024 8:31 AM, Mukesh Kumar Savaliya wrote: > Currently the driver provides a function called geni_serial_resources_off() > to turn off resources like clocks and pinctrl. > > For shared SE between two SS, we don't need to keep pinctrl to sleep state > as other SS may be actively transferring data over SE. Hence,bypass keeping > pinctrl to sleep state conditionally using shared_geni_se flag. > > Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> > --- > drivers/soc/qcom/qcom-geni-se.c | 14 ++++++++++---- > include/linux/soc/qcom/geni-se.h | 3 +++ > 2 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > index 2e8f24d5da80..89cf18699336 100644 > --- a/drivers/soc/qcom/qcom-geni-se.c > +++ b/drivers/soc/qcom/qcom-geni-se.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. > > /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */ > #define __DISABLE_TRACE_MMIO__ > @@ -503,10 +504,15 @@ int geni_se_resources_off(struct geni_se *se) > > if (has_acpi_companion(se->dev)) > return 0; > - > - ret = pinctrl_pm_select_sleep_state(se->dev); > - if (ret) > - return ret; > + /* Keep pinctrl to sleep state only for regular usecase. > + * Do not sleep pinctrl for shared SE because other SS(subsystems) > + * may continueto perform transfer. > + */ /* * Don't alter pin states on shared SEs to avoid potentially * interrupting transfers started by other subsystems */ > + if (se->shared_geni_se == false) { if (!se->shared_geni_se) > + ret = pinctrl_pm_select_sleep_state(se->dev); > + if (ret) > + return ret; > + } > > geni_se_clks_off(se); Should the clocks be turned off? Konrad
This Series adds support to share QUP (Qualcomm Unified peripheral) based I2C SE (Serial Engine) controller between two subsystems (Apps/ADSP/TZ etc). Each SS having it's own dedicated GPII(General Purpose Interface Instance) acting as pipe between SE and GSI(Generic SW- Interface) DMA HW engine. Hence the sharing of SE is possible only with I2C SE in GSI mode, not with FIFO/CPU DMA mode. Subsystem must acquire Lock over the SE so that it gets uninterrupted control till it unlocks the SE. It also makes sure the commonly shared TLMM GPIOs are not touched which can impact other subsystem or cause any interruption. Generally, GPIOs are being unconfigured during suspend time. GSI DMA engine is capable to perform requested transfer operations from any of the I2C client in a seamless way and its transparent to the subsystems. Make sure to enable “qcom,shared-se” flag only while enabling this feature. I2C client should add in its respective parent node. Example : Two clients from different SS can share an I2C SE for same slave device OR their owned slave devices. Assume I2C Slave EEPROM device connected with I2C controller. Each client from ADSP SS and APPS Linux SS can perform i2c transactions. This gets serialized by lock TRE + Transfers + Unlock TRE at HW level. Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com> --- Link to V2: https://lore.kernel.org/lkml/a88a16ff-3537-4396-b2ea-4ba02b4850e9@quicinc.com/T/ Changes in V3: - Added missing maintainers which i forgot to add. - Add cover letter with description of SS and EE for dt-bindings patch. - Added acronyms expansion to commit log. - [PATCH v2 3/4] : Removed exported symbol geni_se_clks_off(). Instead added changes to bypass pinctrl sleep configuration from geni_se_resources_off() function. - Changed title name of [PATCH v2 3/4] to reflect the suggested changes. - [PATCH v2 4/4] kept geni_i2c_runtime_suspend() as is and removed explicit call to geni_se_clks_off(). - Removed is_shared variable from i2c driver and instead used common shared_geni_se variable from qcom-geni-se.h so that other protocols can also extend for similar feature. - I2C driver log changed from dev_err() to dev_dbg() for timeout. - set gpi_mode = true if shared_geni_se is set for this usecase. Enhanced comments around code and commit log. --- Link to V1: https://lore.kernel.org/lkml/cb7613d0-586e-4089-a1b6-2405f4dc4883@quicinc.com/T/ Changes in V2: - Enhanced commit log grammatically for PATCH v1 3/4 as suggested by Bryan. - Updated Cover letter along with acronyms expansion. - Added maintainers list from other subsystems for review, which was missing. Thanks to Krzysztof for pointing out. - Added cover letter with an example of Serial Engine sharing. - Addressed review comments for all the patches. --- Mukesh Kumar Savaliya (4): dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag dma: gpi: Add Lock and Unlock TRE support to access SE exclusively soc: qcom: geni-se: Do not keep GPIOs to sleep state for shared SE usecase i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems .../bindings/i2c/qcom,i2c-geni-qcom.yaml | 4 ++ drivers/dma/qcom/gpi.c | 37 ++++++++++++++++++- drivers/i2c/busses/i2c-qcom-geni.c | 24 ++++++++++-- drivers/soc/qcom/qcom-geni-se.c | 14 +++++-- include/linux/dma/qcom-gpi-dma.h | 6 +++ include/linux/soc/qcom/geni-se.h | 3 ++ 6 files changed, 79 insertions(+), 9 deletions(-)