Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers

[neil.armstrong@linaro.org]

On 11/04/2024 10:46, Vladimir Zapolskiy wrote:
> On 4/10/24 18:26, neil.armstrong@linaro.org wrote:
>> On 10/04/2024 17:19, Vladimir Zapolskiy wrote:
>>> Hi Neil,
>>>
>>> On 4/10/24 16:50, neil.armstrong@linaro.org wrote:
>>>> On 10/04/2024 15:11, Vladimir Zapolskiy wrote:
>>>>> On 4/10/24 10:52, Neil Armstrong wrote:
>>>>>> Hi,
>>>>>>
>>>>>> On 10/04/2024 09:49, Vladimir Zapolskiy wrote:
>>>>>>> Qualcomm SM8650 SoC has three CCI controllers with two I2C busses
>>>>>>> connected to each of them.
>>>>>>>
>>>>>>> The CCI controllers on SM8650 are compatible with the ones found on
>>>>>>> many other older generations of Qualcomm SoCs.
>>>>>>>
>>>>>>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>>>>>>> ---
>>>>>>> The change is based and depends on a patch series from Jagadeesh Kona:
>>>>>>>
>>>>>>>       https://lore.kernel.org/linux-arm-msm/20240321092529.13362-1-quic_jkona@quicinc.com/
>>>>>>>
>>>>>>> It might be an option to add this change right to the series,
>>>>>>> since it anyway requires a respin.
>>>>>>>
>>>>>>> A new compatible value "qcom,sm8650-cci" is NOT added to
>>>>>>> Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml , because
>>>>>>> the controller IP description and selection is covered by a generic
>>>>>>> compatible value "qcom,msm8996-cci".
>>>>>>
>>>>>> You'll still need to add qcom,sm8650-cci to the "CCI v2" list in qcom,i2c-cci.yaml,
>>>>>> otherwise the DTBS check fail, even if the fallback is already present.
>>>>>
>>>>> I do recognize the problem related to a build time warning, my motivation was
>>>>> to follow the rationale described in commit 3e383dce513f
>>>>> ("Revert "dt-bindings: i2c: qcom-cci: Document sc8280xp compatible"").
>>>>>
>>>>> For a similar sc8280xp-cci case it was asked by Konrad to drop a new
>>>>> compatible, I kindly ask the reviewers and maintainers to stick to one
>>>>> of the two contradicting asks.
>>>>
>>>> This is totally different, this commit added a new compatible that is used in the driver,
>>>> while here, you use a per-soc compatible that is (for now), only used in DT and uses
>>>
>>> I'm confused, please elaborate what do you mean above by "this commit" and "here".
>>> Could you please be more specific to avoid any possible disambiguation?
>>
>> "this" refer to "dt-bindings: i2c: qcom-cci: Document sc8280xp compatible".
>>
>>> If you refer to the driver drivers/i2c/busses/i2c-qcom-cci.c, then there is no
>>> difference between sc8280xp-cci and sm8650-cci. What is the total difference,
>>> which you found?
>>
>> If there's no difference between sc8280xp-cci and sm8650-cci, then the policy says
>> you need to _not_ add a new compatible in the driver, which is what you did here.
>>
>>>
>>>> the generic "qcom,msm8996-cci" as a fallback because it is considered as beeing 99%
>>>> compatible and no software change is needed.
>>>>
>>>
>>> I have no objections to revert a "Revert "dt-bindings: i2c: qcom-cci: Document sc8280xp compatible""
>>> commit and to update the change for sm8650-cci accordingly, but as I've
>>> already said it would be good to have and follow one common approach for both
>>> cases, since I based my change on the maintainer's decision from the past.
>>
>> The "new" policy is to use a fallback of an already defined compatible if no driver change
>> is needed, this is the case for the last year so far.
>> And updating the yaml bindings for the new per-soc compatible is also a year-old
>> policy, upstreaming of SM8550, SM8650 and X1E80100 have been done following this policy
> 
> I'm sorry, I'm still failing to understand it, it's trivial to check that there is no
> "sc8280xp-cci" in Documentation/devicetree/bindings/i2c/qcom,i2c-cci.yaml description.
> 
> Despite my multiple asks I did not get an answer from anybody, if commit 3e383dce513f
> ("Revert "dt-bindings: i2c: qcom-cci: Document sc8280xp compatible"") is wrong and
> shall be reverted or not.
> 
> Since my point of discussion is all about the commit 3e383dce513f, because sm8650-cci
> change is based on it, I hope that my original understanding that commit 3e383dce513f
> shall be reverted, I'll send the change shorty.

No 3e383dce513f shall not be reverted, this has nothing to do with sm8650, just send a patch
adding the qcom,sm8650-cci entry in qcom,i2c-cci.yaml following the latest version Bryan did:
https://lore.kernel.org/all/20240111-linux-next-24-01-02-sc8280xp-camss-core-dtsi-v4-0-cdd5c57ff1dc@linaro.org/

It was probably reverted because the if/properties/contains for clock/clock-names settings was missing,
and you should probably do the name otherwise the dtbs_check would fail.

Neil

> 
>> in order to :
>> 1) reduce useless driver changes
>> 2) have a fully verifiable DT against bindings, so we can ensure the DT is 100% valid against the bindings
>>
>