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

[Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>]

Thanks for the review konrad !

On 11/16/2024 12:58 AM, Konrad Dybcio wrote:
> On 13.11.2024 5:14 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.
> 
> Can we read back some value from the registers to know whether such sharing
> takes place?
Actually, HW register doesn't provide such mechanism, it's add on 
feature if SE is programmed for GSI mode.
> 
>> 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.
> 
> The DT is to be taken authoritatively
> 
To clarify - Does it mean i should not have SW disable this feature OR 
override  ? And let it continue in non GSI mode even it's not going to 
work ?
>>
>> 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>
>> ---
> 
> [...]
> 
>>   	} else {
>>   		gi2c->gpi_mode = false;
>> +
>> +		/* Force disable shared SE case for non GSI mode */
>> +		gi2c->se.shared_geni_se = false;
> 
> Doing this silently sounds rather odd..
we can have Some SW logging added ?
> 
> Konrad