[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2e46be46-a029-4b2c-9608-95cb3e18294f@quicinc.com>
Date: Thu, 5 Sep 2024 14:58:38 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: <neil.armstrong@...aro.org>, <konrad.dybcio@...aro.org>,
<andersson@...nel.org>, <andi.shyti@...nel.org>,
<linux-arm-msm@...r.kernel.org>, <dmaengine@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-i2c@...r.kernel.org>
CC: <quic_vdadhani@...cinc.com>
Subject: Re: [PATCH v1 0/4] Enable shared SE support over I2C
Thanks Neil !
On 9/5/2024 12:39 PM, neil.armstrong@...aro.org wrote:
> Hi,
>
> On 04/09/2024 20:07, Mukesh Kumar Savaliya wrote:
>> Thanks Neil !
>>
>> On 8/30/2024 1:17 PM, neil.armstrong@...aro.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
>
Powered by blists - more mailing lists