* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 7:49 [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers Vladimir Zapolskiy
@ 2024-04-10 7:52 ` Neil Armstrong
2024-04-10 13:11 ` Vladimir Zapolskiy
2024-04-10 8:26 ` Krzysztof Kozlowski
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Neil Armstrong @ 2024-04-10 7:52 UTC (permalink / raw)
To: Vladimir Zapolskiy, Bjorn Andersson, Konrad Dybcio,
Jagadeesh Kona
Cc: Krzysztof Kozlowski, linux-arm-msm, devicetree
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.
Neil
>
> arch/arm64/boot/dts/qcom/sm8650.dtsi | 315 +++++++++++++++++++++++++++
> 1 file changed, 315 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index b406835b2e71..160b618dff9c 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -3122,6 +3122,114 @@ videocc: clock-controller@aaf0000 {
> #power-domain-cells = <1>;
> };
>
> + cci0: cci@ac15000 {
> + compatible = "qcom,sm8650-cci", "qcom,msm8996-cci";
> + reg = <0 0x0ac15000 0 0x1000>;
> + interrupts = <GIC_SPI 426 IRQ_TYPE_EDGE_RISING>;
> + power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> + clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
> + <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> + <&camcc CAM_CC_CPAS_AHB_CLK>,
> + <&camcc CAM_CC_CCI_0_CLK>;
> + clock-names = "camnoc_axi",
> + "slow_ahb_src",
> + "cpas_ahb",
> + "cci";
> + pinctrl-0 = <&cci0_default &cci1_default>;
> + pinctrl-1 = <&cci0_sleep &cci1_sleep>;
> + pinctrl-names = "default", "sleep";
> + status = "disabled";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + assigned-clocks = <&camcc CAM_CC_CCI_0_CLK_SRC>;
> + assigned-clock-rates = <37500000>;
> +
> + cci0_i2c0: i2c-bus@0 {
> + reg = <0>;
> + clock-frequency = <400000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> +
> + cci0_i2c1: i2c-bus@1 {
> + reg = <1>;
> + clock-frequency = <400000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> + };
> +
> + cci1: cci@ac16000 {
> + compatible = "qcom,sm8650-cci", "qcom,msm8996-cci";
> + reg = <0 0x0ac16000 0 0x1000>;
> + interrupts = <GIC_SPI 427 IRQ_TYPE_EDGE_RISING>;
> + power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> + clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
> + <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> + <&camcc CAM_CC_CPAS_AHB_CLK>,
> + <&camcc CAM_CC_CCI_1_CLK>;
> + clock-names = "camnoc_axi",
> + "slow_ahb_src",
> + "cpas_ahb",
> + "cci";
> + pinctrl-0 = <&cci2_default &cci3_default>;
> + pinctrl-1 = <&cci2_sleep &cci3_sleep>;
> + pinctrl-names = "default", "sleep";
> + status = "disabled";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cci1_i2c0: i2c-bus@0 {
> + reg = <0>;
> + clock-frequency = <1000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> +
> + cci1_i2c1: i2c-bus@1 {
> + reg = <1>;
> + clock-frequency = <1000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> + };
> +
> + cci2: cci@ac17000 {
> + compatible = "qcom,sm8650-cci", "qcom,msm8996-cci";
> + reg = <0 0x0ac17000 0 0x1000>;
> + interrupts = <GIC_SPI 428 IRQ_TYPE_EDGE_RISING>;
> + power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> + clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
> + <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> + <&camcc CAM_CC_CPAS_AHB_CLK>,
> + <&camcc CAM_CC_CCI_2_CLK>;
> + clock-names = "camnoc_axi",
> + "slow_ahb_src",
> + "cpas_ahb",
> + "cci";
> + pinctrl-0 = <&cci4_default &cci5_default>;
> + pinctrl-1 = <&cci4_sleep &cci5_sleep>;
> + pinctrl-names = "default", "sleep";
> + status = "disabled";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + cci2_i2c0: i2c-bus@0 {
> + reg = <0>;
> + clock-frequency = <1000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> +
> + cci2_i2c1: i2c-bus@1 {
> + reg = <1>;
> + clock-frequency = <1000000>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + };
> + };
> +
> camcc: clock-controller@ade0000 {
> compatible = "qcom,sm8650-camcc";
> reg = <0 0x0ade0000 0 0x20000>;
> @@ -3815,6 +3923,213 @@ tlmm: pinctrl@f100000 {
>
> wakeup-parent = <&pdc>;
>
> + cci0_default: cci0-default-state {
> + sda-pins {
> + pins = "gpio113";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> +
> + scl-pins {
> + pins = "gpio114";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> + };
> +
> + cci0_sleep: cci0-sleep-state {
> + sda-pins {
> + pins = "gpio113";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +
> + scl-pins {
> + pins = "gpio114";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> + };
> +
> + cci1_default: cci1-default-state {
> + sda-pins {
> + pins = "gpio115";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> +
> + scl-pins {
> + pins = "gpio116";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> +
> + mclk-pins {
> + pins = "gpio101";
> + function = "cam_mclk";
> + drive-strength = <2>;
> + bias-disable;
> + };
> +
> + rst-pins {
> + pins = "gpio15";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-disable;
> + output-low;
> + };
> + };
> +
> + cci1_sleep: cci1-sleep-state {
> + sda-pins {
> + pins = "gpio115";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +
> + scl-pins {
> + pins = "gpio116";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> + };
> +
> + cci2_default: cci2-default-state {
> + sda-pins {
> + pins = "gpio117";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> +
> + scl-pins {
> + pins = "gpio118";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> + };
> +
> + cci2_sleep: cci2-sleep-state {
> + sda-pins {
> + pins = "gpio117";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +
> + scl-pins {
> + pins = "gpio118";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> + };
> +
> + cci3_default: cci3-default-state {
> + sda-pins {
> + pins = "gpio12";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> +
> + scl-pins {
> + pins = "gpio13";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> + };
> +
> + cci3_sleep: cci3-sleep-state {
> + sda-pins {
> + pins = "gpio12";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +
> + scl-pins {
> + pins = "gpio13";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> + };
> +
> + cci4_default: cci4-default-state {
> + sda-pins {
> + pins = "gpio112";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> +
> + scl-pins {
> + pins = "gpio153";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> + };
> +
> + cci4_sleep: cci4-sleep-state {
> + sda-pins {
> + pins = "gpio112";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +
> + scl-pins {
> + pins = "gpio153";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> + };
> +
> + cci5_default: cci5-default-state {
> + sda-pins {
> + pins = "gpio119";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> +
> + scl-pins {
> + pins = "gpio120";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-up = <2200>;
> + };
> + };
> +
> + cci5_sleep: cci5-sleep-state {
> + sda-pins {
> + pins = "gpio119";
> + function = "cci_i2c_sda";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> +
> + scl-pins {
> + pins = "gpio120";
> + function = "cci_i2c_scl";
> + drive-strength = <2>;
> + bias-pull-down;
> + };
> + };
> +
> hub_i2c0_data_clk: hub-i2c0-data-clk-state {
> /* SDA, SCL */
> pins = "gpio64", "gpio65";
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 7:52 ` Neil Armstrong
@ 2024-04-10 13:11 ` Vladimir Zapolskiy
2024-04-10 13:50 ` neil.armstrong
0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Zapolskiy @ 2024-04-10 13:11 UTC (permalink / raw)
To: neil.armstrong, Bjorn Andersson, Konrad Dybcio, Jagadeesh Kona
Cc: Krzysztof Kozlowski, linux-arm-msm, devicetree
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.
--
Best wishes,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 13:11 ` Vladimir Zapolskiy
@ 2024-04-10 13:50 ` neil.armstrong
2024-04-10 15:19 ` Vladimir Zapolskiy
0 siblings, 1 reply; 15+ messages in thread
From: neil.armstrong @ 2024-04-10 13:50 UTC (permalink / raw)
To: Vladimir Zapolskiy, Bjorn Andersson, Konrad Dybcio,
Jagadeesh Kona
Cc: Krzysztof Kozlowski, linux-arm-msm, devicetree
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
the generic "qcom,msm8996-cci" as a fallback because it is considered as beeing 99%
compatible and no software change is needed.
But having a per-soc compatible can help adding software support later in the case you
want to implement sm8650 specific features or add bug fixes.
It avoids changing the driver for no reason, and provides a backup in case there's
a driver change needed in the future that can be backported safely and still work
with older DT without changing the DT.
Neil
>
> --
> Best wishes,
> Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 13:50 ` neil.armstrong
@ 2024-04-10 15:19 ` Vladimir Zapolskiy
2024-04-10 15:26 ` neil.armstrong
0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Zapolskiy @ 2024-04-10 15:19 UTC (permalink / raw)
To: neil.armstrong, Bjorn Andersson, Konrad Dybcio, Jagadeesh Kona
Cc: Krzysztof Kozlowski, linux-arm-msm, devicetree
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?
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?
> 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.
--
Best wishes,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 15:19 ` Vladimir Zapolskiy
@ 2024-04-10 15:26 ` neil.armstrong
2024-04-11 8:46 ` Vladimir Zapolskiy
0 siblings, 1 reply; 15+ messages in thread
From: neil.armstrong @ 2024-04-10 15:26 UTC (permalink / raw)
To: Vladimir Zapolskiy, Bjorn Andersson, Konrad Dybcio,
Jagadeesh Kona
Cc: Krzysztof Kozlowski, linux-arm-msm, devicetree
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
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
Neil
>
> --
> Best wishes,
> Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 15:26 ` neil.armstrong
@ 2024-04-11 8:46 ` Vladimir Zapolskiy
2024-04-11 9:57 ` neil.armstrong
0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Zapolskiy @ 2024-04-11 8:46 UTC (permalink / raw)
To: neil.armstrong, Bjorn Andersson, Konrad Dybcio, Jagadeesh Kona
Cc: Krzysztof Kozlowski, linux-arm-msm, devicetree
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.
> 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
>
--
Best wishes,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-11 8:46 ` Vladimir Zapolskiy
@ 2024-04-11 9:57 ` neil.armstrong
0 siblings, 0 replies; 15+ messages in thread
From: neil.armstrong @ 2024-04-11 9:57 UTC (permalink / raw)
To: Vladimir Zapolskiy, Bjorn Andersson, Konrad Dybcio,
Jagadeesh Kona
Cc: Krzysztof Kozlowski, linux-arm-msm, devicetree
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
>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 7:49 [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers Vladimir Zapolskiy
2024-04-10 7:52 ` Neil Armstrong
@ 2024-04-10 8:26 ` Krzysztof Kozlowski
2024-04-10 13:22 ` Vladimir Zapolskiy
2024-04-11 10:01 ` Bryan O'Donoghue
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Krzysztof Kozlowski @ 2024-04-10 8:26 UTC (permalink / raw)
To: Vladimir Zapolskiy, Bjorn Andersson, Konrad Dybcio,
Jagadeesh Kona
Cc: Krzysztof Kozlowski, Neil Armstrong, linux-arm-msm, devicetree
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".
I do not understand this reasoning. So you introduce known errors
because errors are ok?
How does it pass dtbs_check validation?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 8:26 ` Krzysztof Kozlowski
@ 2024-04-10 13:22 ` Vladimir Zapolskiy
2024-04-10 15:45 ` Krzysztof Kozlowski
0 siblings, 1 reply; 15+ messages in thread
From: Vladimir Zapolskiy @ 2024-04-10 13:22 UTC (permalink / raw)
To: Krzysztof Kozlowski, Bjorn Andersson, Konrad Dybcio,
Jagadeesh Kona
Cc: Krzysztof Kozlowski, Neil Armstrong, linux-arm-msm, devicetree
On 4/10/24 11:26, Krzysztof Kozlowski wrote:
> 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".
>
> I do not understand this reasoning. So you introduce known errors
> because errors are ok?
>
> How does it pass dtbs_check validation?
To continue the technical discussion let me ask you to comment on the
absolutely identical subject, which has been taken in the past in connection
to "qcom,sc8280xp-cci" compatible, probably it didn't attact any sufficient
attention before, so let's continue now.
https://lore.kernel.org/linux-arm-msm/0a3cd2f3-85e9-4769-9749-62353e842625@linaro.org/
--
Best wishes,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 13:22 ` Vladimir Zapolskiy
@ 2024-04-10 15:45 ` Krzysztof Kozlowski
0 siblings, 0 replies; 15+ messages in thread
From: Krzysztof Kozlowski @ 2024-04-10 15:45 UTC (permalink / raw)
To: Vladimir Zapolskiy, Bjorn Andersson, Konrad Dybcio,
Jagadeesh Kona
Cc: Krzysztof Kozlowski, Neil Armstrong, linux-arm-msm, devicetree
On 10/04/2024 15:22, Vladimir Zapolskiy wrote:
> On 4/10/24 11:26, Krzysztof Kozlowski wrote:
>> 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".
>>
>> I do not understand this reasoning. So you introduce known errors
>> because errors are ok?
>>
>> How does it pass dtbs_check validation?
>
> To continue the technical discussion let me ask you to comment on the
> absolutely identical subject, which has been taken in the past in connection
> to "qcom,sc8280xp-cci" compatible, probably it didn't attact any sufficient
> attention before, so let's continue now.
>
> https://lore.kernel.org/linux-arm-msm/0a3cd2f3-85e9-4769-9749-62353e842625@linaro.org/
You mix topics. First, you cannot send patch which knowingly introduces
errors, regardless these are build errors or dtbs_check errors.
Plus checkpatch also complains about it.
Second, you linked to a driver discussion, but we talk here about
bindings. Not driver. Each binding must be documented.
Now, about driver, there is no single point nor need to add there
duplicated entry, that's why we don't add.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 7:49 [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers Vladimir Zapolskiy
2024-04-10 7:52 ` Neil Armstrong
2024-04-10 8:26 ` Krzysztof Kozlowski
@ 2024-04-11 10:01 ` Bryan O'Donoghue
2024-04-11 10:19 ` Bryan O'Donoghue
2024-04-15 23:34 ` Konrad Dybcio
4 siblings, 0 replies; 15+ messages in thread
From: Bryan O'Donoghue @ 2024-04-11 10:01 UTC (permalink / raw)
To: Vladimir Zapolskiy, Bjorn Andersson, Konrad Dybcio,
Jagadeesh Kona
Cc: Krzysztof Kozlowski, Neil Armstrong, linux-arm-msm, devicetree
On 10/04/2024 08: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".
>
> arch/arm64/boot/dts/qcom/sm8650.dtsi | 315 +++++++++++++++++++++++++++
> 1 file changed, 315 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> index b406835b2e71..160b618dff9c 100644
> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
> @@ -3122,6 +3122,114 @@ videocc: clock-controller@aaf0000 {
> #power-domain-cells = <1>;
> };
>
> + cci0: cci@ac15000 {
> + compatible = "qcom,sm8650-cci", "qcom,msm8996-cci";
> + reg = <0 0x0ac15000 0 0x1000>;
> + interrupts = <GIC_SPI 426 IRQ_TYPE_EDGE_RISING>;
> + power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> + clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
> + <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> + <&camcc CAM_CC_CPAS_AHB_CLK>,
If you are taking the CPAS_AHB_CLK you don't need to take
SLOW_AHB_CLK_SRC since SLOW_AHB_CLK_SRC is the parent clock of CPAS_AHB_CLK.
https://lore.kernel.org/linux-arm-msm/20240321092529.13362-6-quic_jkona@quicinc.com/
static struct clk_branch cam_cc_cpas_ahb_clk = {};
CPAS_AHB_CLK will get you the AHB clock you need on its own.
Same comment for cci1, cci2.
Other than that LGTM, provided you resolve the compat string stuff - add.
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
bod
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 7:49 [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers Vladimir Zapolskiy
` (2 preceding siblings ...)
2024-04-11 10:01 ` Bryan O'Donoghue
@ 2024-04-11 10:19 ` Bryan O'Donoghue
2024-04-15 23:34 ` Konrad Dybcio
4 siblings, 0 replies; 15+ messages in thread
From: Bryan O'Donoghue @ 2024-04-11 10:19 UTC (permalink / raw)
To: Vladimir Zapolskiy, Bjorn Andersson, Konrad Dybcio,
Jagadeesh Kona
Cc: Krzysztof Kozlowski, Neil Armstrong, linux-arm-msm, devicetree
On 10/04/2024 08:49, Vladimir Zapolskiy wrote:
> + clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
> + <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> + <&camcc CAM_CC_CPAS_AHB_CLK>,
> + <&camcc CAM_CC_CCI_0_CLK>;
> + clock-names = "camnoc_axi",
> + "slow_ahb_src",
> + "cpas_ahb",
> + "cci";
So..
You still need a new compat for this clock list right ?
Current options for clocks which need camnoc_axi are
- if:
properties:
compatible:
contains:
enum:
- qcom,sdm845-cci
- qcom,sm6350-cci
then:
properties:
clocks:
minItems: 6
clock-names:
items:
- const: camnoc_axi
- const: soc_ahb
- const: slow_ahb_src
- const: cpas_ahb
- const: cci
- const: cci_src
and
- if:
properties:
compatible:
contains:
enum:
- qcom,sc7280-cci
- qcom,sm8250-cci
- qcom,sm8450-cci
then:
properties:
clocks:
minItems: 5
maxItems: 5
clock-names:
items:
- const: camnoc_axi
- const: slow_ahb_src
- const: cpas_ahb
- const: cci
- const: cci_src
Also suggest you should look into dropping
slow_ahb_src
cci_src
May not be technically feasible though I accept.
---
bod
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-10 7:49 [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers Vladimir Zapolskiy
` (3 preceding siblings ...)
2024-04-11 10:19 ` Bryan O'Donoghue
@ 2024-04-15 23:34 ` Konrad Dybcio
2024-04-18 21:42 ` Vladimir Zapolskiy
4 siblings, 1 reply; 15+ messages in thread
From: Konrad Dybcio @ 2024-04-15 23:34 UTC (permalink / raw)
To: Vladimir Zapolskiy, Bjorn Andersson, Jagadeesh Kona
Cc: Krzysztof Kozlowski, Neil Armstrong, linux-arm-msm, devicetree
On 4/10/24 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>
> ---
[...]
>
> + cci0: cci@ac15000 {
> + compatible = "qcom,sm8650-cci", "qcom,msm8996-cci";
> + reg = <0 0x0ac15000 0 0x1000>;
> + interrupts = <GIC_SPI 426 IRQ_TYPE_EDGE_RISING>;
> + power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
> + clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
> + <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
The DT should never ever touch the _SRC clocks directly, especially since
you're referencing the branch downstream of it right below
> + <&camcc CAM_CC_CPAS_AHB_CLK>,
> + <&camcc CAM_CC_CCI_0_CLK>;
> + clock-names = "camnoc_axi",
> + "slow_ahb_src",
> + "cpas_ahb",
> + "cci";
> + pinctrl-0 = <&cci0_default &cci1_default>;
> + pinctrl-1 = <&cci0_sleep &cci1_sleep>;
> + pinctrl-names = "default", "sleep";
> + status = "disabled";
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + assigned-clocks = <&camcc CAM_CC_CCI_0_CLK_SRC>;
> + assigned-clock-rates = <37500000>;
Why?
[...]
> + pinctrl-0 = <&cci2_default &cci3_default>;
> + pinctrl-1 = <&cci2_sleep &cci3_sleep>;
Please stick to a single naming scheme (cciX_Y or csiN)
Konrad
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] arm64: dts: qcom: sm8650: add description of CCI controllers
2024-04-15 23:34 ` Konrad Dybcio
@ 2024-04-18 21:42 ` Vladimir Zapolskiy
0 siblings, 0 replies; 15+ messages in thread
From: Vladimir Zapolskiy @ 2024-04-18 21:42 UTC (permalink / raw)
To: Konrad Dybcio, Bjorn Andersson, Jagadeesh Kona
Cc: Krzysztof Kozlowski, Neil Armstrong, linux-arm-msm, devicetree
Hi Konrad,
thank you for review.
On 4/16/24 02:34, Konrad Dybcio wrote:
>
>
> On 4/10/24 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>
>> ---
>
> [...]
>
>>
>> + cci0: cci@ac15000 {
>> + compatible = "qcom,sm8650-cci", "qcom,msm8996-cci";
>> + reg = <0 0x0ac15000 0 0x1000>;
>> + interrupts = <GIC_SPI 426 IRQ_TYPE_EDGE_RISING>;
>> + power-domains = <&camcc CAM_CC_TITAN_TOP_GDSC>;
>> + clocks = <&camcc CAM_CC_CAMNOC_AXI_NRT_CLK>,
>> + <&camcc CAM_CC_SLOW_AHB_CLK_SRC>,
> The DT should never ever touch the _SRC clocks directly, especially since
> you're referencing the branch downstream of it right below
>
Totally agree, this will go away.
>> + <&camcc CAM_CC_CPAS_AHB_CLK>,
>> + <&camcc CAM_CC_CCI_0_CLK>;
>> + clock-names = "camnoc_axi",
>> + "slow_ahb_src",
>> + "cpas_ahb",
>> + "cci";
>> + pinctrl-0 = <&cci0_default &cci1_default>;
>> + pinctrl-1 = <&cci0_sleep &cci1_sleep>;
>> + pinctrl-names = "default", "sleep";
>> + status = "disabled";
>> + #address-cells = <1>;
>> + #size-cells = <0>;
>> +
>> + assigned-clocks = <&camcc CAM_CC_CCI_0_CLK_SRC>;
>> + assigned-clock-rates = <37500000>;
>
> Why?
That's a leftover for silencing a loud complain in the kernel log on
driver initialization, will remove the frequency assignement, thank you!
> [...]
>
>> + pinctrl-0 = <&cci2_default &cci3_default>;
>> + pinctrl-1 = <&cci2_sleep &cci3_sleep>;
>
> Please stick to a single naming scheme (cciX_Y or csiN)
Agreed. It may meet objections, but my preference is to force cciX_Y naming
scheme, and at some point in future I hope pin controls will enter I2C bus
subnodes.
--
Best wishes,
Vladimir
^ permalink raw reply [flat|nested] 15+ messages in thread