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@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. > 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 >