Message-ID: <8b33f935-04a9-48df-8ea1-f6b98efecb9d@kernel.org>
Date: Tue, 10 Dec 2024 13:05:38 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Konrad Dybcio <konradybcio@...il.com>,
 Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
 Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>, 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, conor+dt@...nel.org, agross@...nel.org,
 devicetree@...r.kernel.org, vkoul@...nel.org, linux@...blig.org,
 dan.carpenter@...aro.org, Frank.Li@....com, konradybcio@...nel.org,
 bryan.odonoghue@...aro.org, krzk+dt@...nel.org, robh@...nel.org
Cc: quic_vdadhani@...cinc.com
Subject: Re: [PATCH v5 1/4] dt-bindindgs: i2c: qcom,i2c-geni: Document shared
 flag

On 10/12/2024 12:53, Krzysztof Kozlowski wrote:
>>>> I'm not sure a single property name+description can fit all possible
>>>> cases here. The hardware being "shared" can mean a number of different
>>>
>>> Existing property does not explain anything more, either. To recap -
>>> this block is SE and property is named "se-shared", so basically it is
>>> equal to just "shared". "shared" is indeed quite vague, so I was
>>> expecting some wider work here.
>>>
>>>
>>>> things, with some blocks having hardware provisions for that, while
>>>> others may have totally none and rely on external mechanisms (e.g.
>>>> a shared memory buffer) to indicate whether an external entity
>>>> manages power to them.
>>>
>>> We have properties for that too. Qualcomm SoCs need once per year for
>>> such shared properties. BAM has two or three. IPA has two. There are
>>> probably even more blocks which I don't remember now.
>>
>> So, the problem is "driver must not toggle GPIO states", because
>> "the bus controller must not be muxed away from the endpoint".
>> You can come up with a number of similar problems by swapping out
>> the quoted text.
>>
>> We can either describe what the driver must do (A), or what the
>> reason for it is (B).
>>
>>
>> If we go with A, we could have a property like:
>>
>> &i2c1 {
>> 	externally-handled-resources = <(EHR_PINCTRL_STATE | EHR_CLOCK_RATE)>
>> };
>>
>> which would be a generic list of things that the OS would have to
>> tiptoe around, fitting Linux's framework split quite well
>>
>>
>>
>> or if we go with B, we could add a property like:
>>
>> &i2c1 {
>> 	qcom,shared-controller;
>> };
>>
>> which would hide the implementation details into the driver
>>
>> I could see both approaches having their place, but in this specific
>> instance I think A would be more fitting, as the problem is quite
>> simple.
> 
> 
> The second is fine with me, maybe missing information about "whom" do
> you share it with. Or maybe we get to the point that all this is
> specific to SoC, thus implied by compatible and we do not need
> downstream approach (another discussion in USB pushed by Qcom: I want
> one compatible and 1000 properties).
> 
> I really wished Qualcomm start reworking their bindings before they are
> being sent upstream to match standard DT guidelines, not downstream
> approach. Somehow these hundreds reviews we give could result in new
> patches doing things better, not just repeating the same issues.

This is BTW v5, with all the same concerns from v1 and still no answers
in commit msg about these concerns. Nothing explained in commit msg
which hardware needs it or why the same SoC have it once shared, once
not (exclusive). Basically there is nothing here corresponding to any
real product, so since five versions all this for me is just copy-paste
from downstream approach.


Best regards,
Krzysztof

Powered by blists - more mailing lists