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

Mukesh Kumar Savaliya
2026-03-31T11:35:06Z

Hi Konrad, Thanks for the review and sorry for long delay replying on
this change. I was completely away from this work.
Let me upload V6 to share latest changes and cover letter details to
help review ahead.

On 12/16/2024 6:17 PM, Mukesh Kumar Savaliya wrote:
>
>
> On 12/16/2024 5:40 PM, Konrad Dybcio wrote:
>> On 15.12.2024 9:59 AM, Mukesh Kumar Savaliya wrote:
>>> 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 ?
>>
>> Yes, drop changing this comment.
> Sure, Thanks for confirming !
Done this change with new flag name addressing the previous comments
from dt-binding file.
>>
>> Konrad
>