Re: [PATCH v5 4/4] i2c: i2c-qcom-geni: Enable i2c controller sharing between two subsystems

Mukesh Kumar Savaliya
2024-12-15T08:59:34Z

Hi Konrad,

On 12/13/2024 6:35 PM, Konrad Dybcio wrote:
> On 29.11.2024 3:43 PM, Mukesh Kumar Savaliya wrote:
>> Add support to share I2C controller in multiprocessor system 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 I2C SE(Serial engine) is possible only for GSI mode as client
>> from each processor 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.
>>
>> For example, Assume an I2C EEPROM device connected with an I2C controller.
>> Each client from ADSP and APPS processor can perform i2c transactions
>> without any disturbance from each other.
>>
>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>> ---
>> drivers/i2c/busses/i2c-qcom-geni.c | 22 +++++++++++++++++++---
>> 1 file changed, 19 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 7a22e1f46e60..ccf9933e2dad 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>
>> @@ -617,6 +618,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];
>> @@ -627,6 +629,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,
>> @@ -815,6 +819,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));
>>
>> @@ -887,8 +896,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 don't think this change makes things clearer, I would drop it
Shall i revert back to previous change ? What's your suggestion ?
>
>> gi2c->gpi_mode = true;
>> ret = setup_gpi_dma(gi2c);
>> if (ret) {
>> @@ -900,6 +911,12 @@ static int geni_i2c_probe(struct platform_device *pdev)
>> dev_dbg(dev, "Using GPI DMA mode for I2C\n");
>> } else {
>> gi2c->gpi_mode = false;
>> +
>> + if (gi2c->se.shared_geni_se) {
>> + dev_err(dev, "I2C sharing is not supported in non GSI mode\n");
>> + return -EINVAL;
>
> return dev_err_probe(dev, -EINVAL, "I2C...)
Yes, will incorporate this change in next patch. dt-binding change i am
seeking something good writeup, then will upload V6. Thanks Konrad !
>
> Konrad