Re: [PATCH v1 0/4] Enable shared SE support over I2C

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

Thanks Neil !

On 9/5/2024 12:39 PM, neil.armstrong@linaro.org wrote:
> Hi,
> 
> On 04/09/2024 20:07, Mukesh Kumar Savaliya wrote:
>> Thanks Neil !
>>
>> On 8/30/2024 1:17 PM, neil.armstrong@linaro.org wrote:
>>> Hi,
>>>
>>> On 29/08/2024 11:24, Mukesh Kumar Savaliya wrote:
>>>> This Series adds support to share QUP based I2C SE between subsystems.
>>>> Each subsystem should have its own GPII which interacts between SE and
>>>> GSI DMA HW engine.
>>>>
>>>> Subsystem must acquire Lock over the SE on GPII channel so that it
>>>> gets uninterrupted control till it unlocks the SE. It also makes sure
>>>> the commonly shared TLMM GPIOs are not touched which can impact other
>>>> subsystem or cause any interruption. Generally, GPIOs are being
>>>> unconfigured during suspend time.
>>>>
>>>> GSI DMA engine is capable to perform requested transfer operations
>>>> from any of the SE in a seamless way and its transparent to the
>>>> subsystems. Make sure to enable “qcom,shared-se” flag only while
>>>> enabling this feature. I2C client should add in its respective parent
>>>> node.
>>>>
>>>> ---
>>>> Mukesh Kumar Savaliya (4):
>>>>    dt-bindindgs: i2c: qcom,i2c-geni: Document shared flag
>>>>    dma: gpi: Add Lock and Unlock TRE support to access SE exclusively
>>>>    soc: qcom: geni-se: Export function geni_se_clks_off()
>>>>    i2c: i2c-qcom-geni: Enable i2c controller sharing between two
>>>>      subsystems
>>>>
>>>>   .../bindings/i2c/qcom,i2c-geni-qcom.yaml      |  4 ++
>>>>   drivers/dma/qcom/gpi.c                        | 37 
>>>> ++++++++++++++++++-
>>>>   drivers/i2c/busses/i2c-qcom-geni.c            | 29 +++++++++++----
>>>>   drivers/soc/qcom/qcom-geni-se.c               |  4 +-
>>>>   include/linux/dma/qcom-gpi-dma.h              |  6 +++
>>>>   include/linux/soc/qcom/geni-se.h              |  3 ++
>>>>   6 files changed, 74 insertions(+), 9 deletions(-)
>>>>
>>>
>>> I see in downstream that this flag is used on the SM8650 qupv3_se6_i2c,
>>> and that on the SM8650-HDK this i2c is shared between the aDSP 
>>> battmgr and
>>> the linux to access the HDMI controller.
>>>
>>> Is this is the target use-case ?
>> Not exactly that usecase. Here making it generic in a way to transfer 
>> data which is pushed from two subsystems independently. Consider for 
>> example one is ADSP i2c client and another is Linux i2c client. Not 
>> sure in what manner battmgr and HDMI sends traffic. we can debug it 
>> separately over that email.
> 
> Considering battmgr runs in ADSP, it matches this use-case, no ?
> 
is your issue 100% ? I have received your email, so will debug over that 
email.
>>>
>>> We have some issues on this platform that crashes the system when Linux
>>> does some I2C transfers while battmgr does some access at the same time,
>>> the problem is that on the Linux side the i2c uses the SE DMA and not 
>>> GPI
>>> because fifo_disable=0 so by default this bypasses GPI.
>>>
>>> A temporary fix has been merged:
>>> https://lore.kernel.org/all/20240605-topic-sm8650-upstream-hdk-iommu-fix-v1-1-9fd7233725fa@linaro.org/
>>> but it's clearly not a real solution
>>>
>> Seems you have added SID for the GPII being used from linux side. Need 
>> to know why you have added it and is it helping ? I have sent an email 
>> to know more about this issue before 2 weeks.
> 
> I've added this because it actually avoids crashing when doing I2C6 
> transactions over SE DMA, now we need to understand why.
> 
Seems stream IS (SID) is corrected and points to the potential wrong 
device tree configuration. Its required for DMA transactions.
>>
>>> What would be the solution to use the shared i2c with on one side 
>>> battmgr
>>> using GPI and the kernel using SE DMA ?
>>>
>> I have already sent an email on this issue, please respond on it. We 
>> shall debug it separately since this feature about sharing is still 
>> under implementation as you know about this patch series.
> 
> Sorry for the delay, I was technically unable to answer, let me resume 
> it now that I'm able again.
> 
Sure, lets discuss there.
> Thanks,
> Neil
> 
>>
>>> In this case, shouldn't we force using GPI on linux with:
>>> ==============><=====================================================================
>>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c 
>>> b/drivers/i2c/busses/i2c-qcom-geni.c
>>> index ee2e431601a6..a15825ea56de 100644
>>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>>> @@ -885,7 +885,7 @@ 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) {
>>> +       if (gi2c->is_shared || fifo_disable) {
>>>                  /* FIFO is disabled, so we can only use GPI DMA */
>>>                  gi2c->gpi_mode = true;
>>>                  ret = setup_gpi_dma(gi2c);
>>> ==============><=====================================================================
>>>
>>> Neil
>