| Message ID | 20240410074951.447898-1-vladimir.zapolskiy@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: qcom: sm8650: add description of CCI controllers | expand |
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";
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
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
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
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 >
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 >> >
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
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
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
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
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";
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(+)