Received-SPF: pass (google.com: domain of linux-kernel+bounces-138496-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <f565a3ae-f46b-46ef-ab6f-2d4fb645a4ec@linaro.org>
Date: Wed, 10 Apr 2024 14:29:50 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] i2c: i2c-qcom-geni: Add support to share an I2C SE
 from two subsystem
To: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>, andersson@kernel.org,
 andi.shyti@kernel.org, linux-arm-msm@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
 dmaengine@vger.kernel.org
Cc: quic_vdadhani@quicinc.com, Vinod Koul <vkoul@kernel.org>
References: <20240402062131.9836-1-quic_msavaliy@quicinc.com>
 <51c84af2-73f7-4af4-8676-2276b6c7786d@linaro.org>
 <669b516a-74c7-445a-b151-5463fe39b21b@quicinc.com>
Content-Language: en-US
From: Konrad Dybcio <konrad.dybcio@linaro.org>
In-Reply-To: <669b516a-74c7-445a-b151-5463fe39b21b@quicinc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 4/3/24 07:56, Mukesh Kumar Savaliya wrote:
> Thanks Konrad. I understood.
> 
> On 4/2/2024 7:54 PM, Konrad Dybcio wrote:
>> On 2.04.2024 8:21 AM, Mukesh Kumar Savaliya wrote:
>>> Add feature to share an I2C serial engine between two subsystems(SS) so
>>> that individual clients from different subsystems can access the same bus.
>>> For example single i2c slave device can be accessed by Client driver from
>>> APPS OR modem subsystem image. Same way we can have slave being accessed
>>> between APPS and TZ subsystems.
>>>
>>> This is possible in GSI mode where driver queues the TREs with required
>>> descriptors and ensures to execute TREs in an mutually exclusive way.
>>> Issue a "Lock TRE" command at the start of the transfer and an "Unlock TRE"
>>> command at the end of the transfer. This prevents other subsystems from
>>> concurrently performing DMA transfers and avoids disturbance to data path.
>>> Change MAX_TRE macro to 5 from 3 because of these two additional TREs.
>>>
>>> Since the GPIOs are also shared for the i2c bus, do not touch GPIO
>>> configuration while going to runtime suspend and only turn off the
>>> clocks. This will allow other SS to continue to transfer the data.
>>>
>>> This feature needs to be controlled by DTSI flag to make it flexible
>>> based on the usecase, hence during probe check the same from i2c driver.
>>>
>>> Export function geni_se_clks_off() to call explicitly instead of
>>> geni_se_resources_off() to not modify TLMM configuration as other SS might
>>> perform the transfer while APPS SS can go to sleep.
>>>
>>> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>
>>> ---
>>> v1 -> v2:
>>> - Addressed review comments.
>>
>> The biggest one ("too many changes across the board") is still not
>> addressed and the patch will not be further reviewed until that is done.
>>
>> Each subsystem has different owners and each change requires an explanation
>> (maintainers always "expect your patch to be wrong" and you need to
>> convince them otherwise through commit messages)
>>
> Sure, I got it. Will send patch dividing logically between i2c, dma.
> I have already responded in just previous Mail to seek clarity as below.
> It was :
> "Please correct me if this is wrong. The overall change is for i2c in GSI DMA mode. This also requires changes in resource control like TLMM changes. But it's more like integrated feature.
> Are you suggesting to make 3 sub-patches under same change ? "

Yes, every subsequent patch can only introduce changes to one subsystem/
driver and can not depend on the next patches (i.e. the order matters)

Konrad