| Message ID | 20240927063108.2773304-5-quic_msavaliy@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable shared SE support over I2C | expand |
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 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
Thanks Bryan for sharing your comments. On 10/15/2024 3:06 AM, Bryan O'Donoghue wrote: > 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 ? > It's like between two processor systems. > For example the APSS - Application Specific Sub Subsystem - where Linux > runs and say cDSP - the compute DSP on qcom SoCs. > Yes > 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" > I think bus masters can be within same APPS system too. hence > 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 ? yes, It's DSP processor here. But can be Low power DSP OR Modem DSP. > > Anyway highlight one use-case for this AMP case, please. I have added below in cover letter. I should add example in this patch also. 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. > > --- > bod > > > >
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@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. 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 >>
Hi Bjorn, On 10/1/2024 8:09 AM, Bjorn Andersson wrote: > 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. Yes, Thats right. Transfer ring element, it's like a command to the engine. Can be configuration TRE, DMA xfer request TRE etc. > >> 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. > I think terminate_sync() should clean up all xfers and will continue for the next request as a fresh start. > 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... > Explicitly adding lock/unlock per transfer adds execution load. Lock is meant for taking an ownership of the bus which is better to manage per session. > Regards, > Bjorn > >> regards, >> dan carpenter >>
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); 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); 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; + 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. + **/ 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 */ + gi2c->se.shared_geni_se = false; 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; - } else { gi2c->suspended = 1; }
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. 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. 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(-)