Message-ID: <aad3c217-a6f6-4415-8e08-8fc113504756@quicinc.com>
Date: Tue, 10 Dec 2024 20:47:46 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Konrad Dybcio <konradybcio@...il.com>,
        Krzysztof Kozlowski
	<krzk@...nel.org>,
        Konrad Dybcio <konrad.dybcio@....qualcomm.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

Thanks Konrad !

On 12/10/2024 6:08 PM, Konrad Dybcio wrote:
> 
> 
> On 12/10/24 13:05, Krzysztof Kozlowski wrote:
>> 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.
> 
> So since this is a software contract and not a hardware
> feature, this is not bound to any specific SoC or "firmware",
> but rather to what runs on other cores (e.g. DSPs, MCUs spread
> across the SoC or in a different software world, like TZ).
> 
> Specifying the specific intended use would be helpful though,
> indeed.
> 
> Let's see if we can somehow make this saner.
> 
> 
> Mukesh, do we have any spare registers that we could use to
> indicate that a given SE is shared? Preferably within the
> SE's register space itself. The bootloader or another entity
> (DSP or what have you) would then set that bit before Linux
> runs and we could skip the bindings story altogether.
> 
There would be spare register but i think it should be in sync with 
hardware team. let me check with them and update back if any bit can be 
repurposed for this feature. I agree, if any register is available, it 
can programmed prior to kernel.
> It would need to be reserved on all SoCs though (future and
> past), to make sure the contract is always held up, but I
> think finding a persistent bit that has never been used
> shouldn't be impossible.
> 
Yes, let me check it with hardware and firmware team and update back. 
Does this mean, there can't be a such software sharing mechanism (purely 
software decision) based on DTSI flag ?
> Konrad

Powered by blists - more mailing lists