| Message ID | 20240321092529.13362-7-quic_jkona@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for videocc and camcc on SM8650 | expand |
Hello Jagadeesh, On 3/21/24 11:25, Jagadeesh Kona wrote: > Add device nodes for video and camera clock controllers on Qualcomm > SM8650 platform. > > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi > index 32c0a7b9aded..d862aa6be824 100644 > --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi > @@ -4,6 +4,8 @@ > */ > > #include <dt-bindings/clock/qcom,rpmh.h> > +#include <dt-bindings/clock/qcom,sm8450-videocc.h> > +#include <dt-bindings/clock/qcom,sm8650-camcc.h> > #include <dt-bindings/clock/qcom,sm8650-dispcc.h> > #include <dt-bindings/clock/qcom,sm8650-gcc.h> > #include <dt-bindings/clock/qcom,sm8650-gpucc.h> > @@ -3110,6 +3112,32 @@ opp-202000000 { > }; > }; > > + videocc: clock-controller@aaf0000 { > + compatible = "qcom,sm8650-videocc"; > + reg = <0 0x0aaf0000 0 0x10000>; > + clocks = <&bi_tcxo_div2>, > + <&gcc GCC_VIDEO_AHB_CLK>; > + power-domains = <&rpmhpd RPMHPD_MMCX>; > + required-opps = <&rpmhpd_opp_low_svs>; Please add default status = "disabled"; > + #clock-cells = <1>; > + #reset-cells = <1>; > + #power-domain-cells = <1>; > + }; > + > + camcc: clock-controller@ade0000 { > + compatible = "qcom,sm8650-camcc"; > + reg = <0 0x0ade0000 0 0x20000>; > + clocks = <&gcc GCC_CAMERA_AHB_CLK>, > + <&bi_tcxo_div2>, > + <&bi_tcxo_ao_div2>, > + <&sleep_clk>; > + power-domains = <&rpmhpd RPMHPD_MMCX>; > + required-opps = <&rpmhpd_opp_low_svs>; Please add default status = "disabled"; > + #clock-cells = <1>; > + #reset-cells = <1>; > + #power-domain-cells = <1>; > + }; > + > mdss: display-subsystem@ae00000 { > compatible = "qcom,sm8650-mdss"; > reg = <0 0x0ae00000 0 0x1000>; After disabling the clock controllers Reviewed-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> -- Best wishes, Vladimir
On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > Add device nodes for video and camera clock controllers on Qualcomm > SM8650 platform. > > Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi > index 32c0a7b9aded..d862aa6be824 100644 > --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi > +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi > @@ -4,6 +4,8 @@ > */ > > #include <dt-bindings/clock/qcom,rpmh.h> > +#include <dt-bindings/clock/qcom,sm8450-videocc.h> > +#include <dt-bindings/clock/qcom,sm8650-camcc.h> > #include <dt-bindings/clock/qcom,sm8650-dispcc.h> > #include <dt-bindings/clock/qcom,sm8650-gcc.h> > #include <dt-bindings/clock/qcom,sm8650-gpucc.h> > @@ -3110,6 +3112,32 @@ opp-202000000 { > }; > }; > > + videocc: clock-controller@aaf0000 { > + compatible = "qcom,sm8650-videocc"; > + reg = <0 0x0aaf0000 0 0x10000>; > + clocks = <&bi_tcxo_div2>, > + <&gcc GCC_VIDEO_AHB_CLK>; > + power-domains = <&rpmhpd RPMHPD_MMCX>; > + required-opps = <&rpmhpd_opp_low_svs>; The required-opps should no longer be necessary. > + #clock-cells = <1>; > + #reset-cells = <1>; > + #power-domain-cells = <1>; > + }; > + > + camcc: clock-controller@ade0000 { > + compatible = "qcom,sm8650-camcc"; > + reg = <0 0x0ade0000 0 0x20000>; > + clocks = <&gcc GCC_CAMERA_AHB_CLK>, > + <&bi_tcxo_div2>, > + <&bi_tcxo_ao_div2>, > + <&sleep_clk>; > + power-domains = <&rpmhpd RPMHPD_MMCX>; > + required-opps = <&rpmhpd_opp_low_svs>; > + #clock-cells = <1>; > + #reset-cells = <1>; > + #power-domain-cells = <1>; > + }; > + > mdss: display-subsystem@ae00000 { > compatible = "qcom,sm8650-mdss"; > reg = <0 0x0ae00000 0 0x1000>; > -- > 2.43.0 > >
On 21.03.2024 14:07, Vladimir Zapolskiy wrote: > Hello Jagadeesh, > > On 3/21/24 11:25, Jagadeesh Kona wrote: >> Add device nodes for video and camera clock controllers on Qualcomm >> SM8650 platform. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi >> index 32c0a7b9aded..d862aa6be824 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >> @@ -4,6 +4,8 @@ >> */ >> #include <dt-bindings/clock/qcom,rpmh.h> >> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> >> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> >> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> >> #include <dt-bindings/clock/qcom,sm8650-gcc.h> >> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> >> @@ -3110,6 +3112,32 @@ opp-202000000 { >> }; >> }; >> + videocc: clock-controller@aaf0000 { >> + compatible = "qcom,sm8650-videocc"; >> + reg = <0 0x0aaf0000 0 0x10000>; >> + clocks = <&bi_tcxo_div2>, >> + <&gcc GCC_VIDEO_AHB_CLK>; >> + power-domains = <&rpmhpd RPMHPD_MMCX>; >> + required-opps = <&rpmhpd_opp_low_svs>; > > Please add default status = "disabled"; > >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + #power-domain-cells = <1>; >> + }; >> + >> + camcc: clock-controller@ade0000 { >> + compatible = "qcom,sm8650-camcc"; >> + reg = <0 0x0ade0000 0 0x20000>; >> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, >> + <&bi_tcxo_div2>, >> + <&bi_tcxo_ao_div2>, >> + <&sleep_clk>; >> + power-domains = <&rpmhpd RPMHPD_MMCX>; >> + required-opps = <&rpmhpd_opp_low_svs>; > > Please add default status = "disabled"; > >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + #power-domain-cells = <1>; >> + }; >> + >> mdss: display-subsystem@ae00000 { >> compatible = "qcom,sm8650-mdss"; >> reg = <0 0x0ae00000 0 0x1000>; > > After disabling the clock controllers Clock controllers should never be disabled period, that defeats the entire point of having unused clk/pd cleanup. The only reason for them to be disabled is for cases where platform crashes on access due to stinky "security" settings (like with audio clocks), or when people are too lazy to upstream panel drivers and end up partially upstreaming display-related changes and continue using the bootloader-initialized framebuffer. This takes away from the very little determinism we have. Konrad
On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: > On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> Add device nodes for video and camera clock controllers on Qualcomm >> SM8650 platform. >> >> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi >> index 32c0a7b9aded..d862aa6be824 100644 >> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >> @@ -4,6 +4,8 @@ >> */ >> >> #include <dt-bindings/clock/qcom,rpmh.h> >> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> >> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> >> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> >> #include <dt-bindings/clock/qcom,sm8650-gcc.h> >> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> >> @@ -3110,6 +3112,32 @@ opp-202000000 { >> }; >> }; >> >> + videocc: clock-controller@aaf0000 { >> + compatible = "qcom,sm8650-videocc"; >> + reg = <0 0x0aaf0000 0 0x10000>; >> + clocks = <&bi_tcxo_div2>, >> + <&gcc GCC_VIDEO_AHB_CLK>; >> + power-domains = <&rpmhpd RPMHPD_MMCX>; >> + required-opps = <&rpmhpd_opp_low_svs>; > > The required-opps should no longer be necessary. > Sure, will check and remove this if not required. Thanks, Jagadeesh >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + #power-domain-cells = <1>; >> + }; >> + >> + camcc: clock-controller@ade0000 { >> + compatible = "qcom,sm8650-camcc"; >> + reg = <0 0x0ade0000 0 0x20000>; >> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, >> + <&bi_tcxo_div2>, >> + <&bi_tcxo_ao_div2>, >> + <&sleep_clk>; >> + power-domains = <&rpmhpd RPMHPD_MMCX>; >> + required-opps = <&rpmhpd_opp_low_svs>; >> + #clock-cells = <1>; >> + #reset-cells = <1>; >> + #power-domain-cells = <1>; >> + }; >> + >> mdss: display-subsystem@ae00000 { >> compatible = "qcom,sm8650-mdss"; >> reg = <0 0x0ae00000 0 0x1000>; >> -- >> 2.43.0 >> >> > >
On 3/25/2024 11:38 AM, Jagadeesh Kona wrote: > > > On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: >> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> >> wrote: >>> >>> Add device nodes for video and camera clock controllers on Qualcomm >>> SM8650 platform. >>> >>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>> --- >>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ >>> 1 file changed, 28 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>> index 32c0a7b9aded..d862aa6be824 100644 >>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>> @@ -4,6 +4,8 @@ >>> */ >>> >>> #include <dt-bindings/clock/qcom,rpmh.h> >>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> >>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> >>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> >>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> >>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> >>> @@ -3110,6 +3112,32 @@ opp-202000000 { >>> }; >>> }; >>> >>> + videocc: clock-controller@aaf0000 { >>> + compatible = "qcom,sm8650-videocc"; >>> + reg = <0 0x0aaf0000 0 0x10000>; >>> + clocks = <&bi_tcxo_div2>, >>> + <&gcc GCC_VIDEO_AHB_CLK>; >>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>> + required-opps = <&rpmhpd_opp_low_svs>; >> >> The required-opps should no longer be necessary. >> > > Sure, will check and remove this if not required. I checked further on this and without required-opps, if there is no vote on the power-domain & its peer from any other consumers, when runtime get is called on device, it enables the power domain just at the minimum non-zero level. But in some cases, the minimum non-zero level of power-domain could be just retention and is not sufficient for clock controller to operate, hence required-opps property is needed to specify the minimum level required on power-domain for this clock controller. Thanks, Jagadeesh > >>> + #clock-cells = <1>; >>> + #reset-cells = <1>; >>> + #power-domain-cells = <1>; >>> + }; >>> + >>> + camcc: clock-controller@ade0000 { >>> + compatible = "qcom,sm8650-camcc"; >>> + reg = <0 0x0ade0000 0 0x20000>; >>> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, >>> + <&bi_tcxo_div2>, >>> + <&bi_tcxo_ao_div2>, >>> + <&sleep_clk>; >>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>> + required-opps = <&rpmhpd_opp_low_svs>; >>> + #clock-cells = <1>; >>> + #reset-cells = <1>; >>> + #power-domain-cells = <1>; >>> + }; >>> + >>> mdss: display-subsystem@ae00000 { >>> compatible = "qcom,sm8650-mdss"; >>> reg = <0 0x0ae00000 0 0x1000>; >>> -- >>> 2.43.0 >>> >>> >> >>
On Wed, 3 Apr 2024 at 10:16, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 3/25/2024 11:38 AM, Jagadeesh Kona wrote: > > > > > > On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: > >> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> > >> wrote: > >>> > >>> Add device nodes for video and camera clock controllers on Qualcomm > >>> SM8650 platform. > >>> > >>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > >>> --- > >>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ > >>> 1 file changed, 28 insertions(+) > >>> > >>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>> index 32c0a7b9aded..d862aa6be824 100644 > >>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>> @@ -4,6 +4,8 @@ > >>> */ > >>> > >>> #include <dt-bindings/clock/qcom,rpmh.h> > >>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> > >>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> > >>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> > >>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> > >>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> > >>> @@ -3110,6 +3112,32 @@ opp-202000000 { > >>> }; > >>> }; > >>> > >>> + videocc: clock-controller@aaf0000 { > >>> + compatible = "qcom,sm8650-videocc"; > >>> + reg = <0 0x0aaf0000 0 0x10000>; > >>> + clocks = <&bi_tcxo_div2>, > >>> + <&gcc GCC_VIDEO_AHB_CLK>; > >>> + power-domains = <&rpmhpd RPMHPD_MMCX>; > >>> + required-opps = <&rpmhpd_opp_low_svs>; > >> > >> The required-opps should no longer be necessary. > >> > > > > Sure, will check and remove this if not required. > > > I checked further on this and without required-opps, if there is no vote > on the power-domain & its peer from any other consumers, when runtime > get is called on device, it enables the power domain just at the minimum > non-zero level. But in some cases, the minimum non-zero level of > power-domain could be just retention and is not sufficient for clock > controller to operate, hence required-opps property is needed to specify > the minimum level required on power-domain for this clock controller. In which cases? If it ends up with the retention vote, it is a bug which must be fixed. > > Thanks, > Jagadeesh > > > > >>> + #clock-cells = <1>; > >>> + #reset-cells = <1>; > >>> + #power-domain-cells = <1>; > >>> + }; > >>> + > >>> + camcc: clock-controller@ade0000 { > >>> + compatible = "qcom,sm8650-camcc"; > >>> + reg = <0 0x0ade0000 0 0x20000>; > >>> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, > >>> + <&bi_tcxo_div2>, > >>> + <&bi_tcxo_ao_div2>, > >>> + <&sleep_clk>; > >>> + power-domains = <&rpmhpd RPMHPD_MMCX>; > >>> + required-opps = <&rpmhpd_opp_low_svs>; > >>> + #clock-cells = <1>; > >>> + #reset-cells = <1>; > >>> + #power-domain-cells = <1>; > >>> + }; > >>> + > >>> mdss: display-subsystem@ae00000 { > >>> compatible = "qcom,sm8650-mdss"; > >>> reg = <0 0x0ae00000 0 0x1000>; > >>> -- > >>> 2.43.0 > >>> > >>> > >> > >>
On 4/3/2024 9:24 PM, Dmitry Baryshkov wrote: > On Wed, 3 Apr 2024 at 10:16, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 3/25/2024 11:38 AM, Jagadeesh Kona wrote: >>> >>> >>> On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: >>>> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> >>>> wrote: >>>>> >>>>> Add device nodes for video and camera clock controllers on Qualcomm >>>>> SM8650 platform. >>>>> >>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>>>> --- >>>>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ >>>>> 1 file changed, 28 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>> index 32c0a7b9aded..d862aa6be824 100644 >>>>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>> @@ -4,6 +4,8 @@ >>>>> */ >>>>> >>>>> #include <dt-bindings/clock/qcom,rpmh.h> >>>>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> >>>>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> >>>>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> >>>>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> >>>>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> >>>>> @@ -3110,6 +3112,32 @@ opp-202000000 { >>>>> }; >>>>> }; >>>>> >>>>> + videocc: clock-controller@aaf0000 { >>>>> + compatible = "qcom,sm8650-videocc"; >>>>> + reg = <0 0x0aaf0000 0 0x10000>; >>>>> + clocks = <&bi_tcxo_div2>, >>>>> + <&gcc GCC_VIDEO_AHB_CLK>; >>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>>>> + required-opps = <&rpmhpd_opp_low_svs>; >>>> >>>> The required-opps should no longer be necessary. >>>> >>> >>> Sure, will check and remove this if not required. >> >> >> I checked further on this and without required-opps, if there is no vote >> on the power-domain & its peer from any other consumers, when runtime >> get is called on device, it enables the power domain just at the minimum >> non-zero level. But in some cases, the minimum non-zero level of >> power-domain could be just retention and is not sufficient for clock >> controller to operate, hence required-opps property is needed to specify >> the minimum level required on power-domain for this clock controller. > > In which cases? If it ends up with the retention vote, it is a bug > which must be fixed. > The minimum non-zero level(configured from bootloaders) of MMCX is retention on few chipsets but it can vary across the chipsets. Hence to be on safer side from our end, it is good to have required-opps in DT to specify the minimum level required for this clock controller. Thanks, Jagadeesh >> >> Thanks, >> Jagadeesh >> >>> >>>>> + #clock-cells = <1>; >>>>> + #reset-cells = <1>; >>>>> + #power-domain-cells = <1>; >>>>> + }; >>>>> + >>>>> + camcc: clock-controller@ade0000 { >>>>> + compatible = "qcom,sm8650-camcc"; >>>>> + reg = <0 0x0ade0000 0 0x20000>; >>>>> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, >>>>> + <&bi_tcxo_div2>, >>>>> + <&bi_tcxo_ao_div2>, >>>>> + <&sleep_clk>; >>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>>>> + required-opps = <&rpmhpd_opp_low_svs>; >>>>> + #clock-cells = <1>; >>>>> + #reset-cells = <1>; >>>>> + #power-domain-cells = <1>; >>>>> + }; >>>>> + >>>>> mdss: display-subsystem@ae00000 { >>>>> compatible = "qcom,sm8650-mdss"; >>>>> reg = <0 0x0ae00000 0 0x1000>; >>>>> -- >>>>> 2.43.0 >>>>> >>>>> >>>> >>>> > > >
On Thu, 4 Apr 2024 at 08:13, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 4/3/2024 9:24 PM, Dmitry Baryshkov wrote: > > On Wed, 3 Apr 2024 at 10:16, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >> > >> > >> > >> On 3/25/2024 11:38 AM, Jagadeesh Kona wrote: > >>> > >>> > >>> On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: > >>>> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> > >>>> wrote: > >>>>> > >>>>> Add device nodes for video and camera clock controllers on Qualcomm > >>>>> SM8650 platform. > >>>>> > >>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > >>>>> --- > >>>>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ > >>>>> 1 file changed, 28 insertions(+) > >>>>> > >>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>> index 32c0a7b9aded..d862aa6be824 100644 > >>>>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>> @@ -4,6 +4,8 @@ > >>>>> */ > >>>>> > >>>>> #include <dt-bindings/clock/qcom,rpmh.h> > >>>>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> > >>>>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> > >>>>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> > >>>>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> > >>>>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> > >>>>> @@ -3110,6 +3112,32 @@ opp-202000000 { > >>>>> }; > >>>>> }; > >>>>> > >>>>> + videocc: clock-controller@aaf0000 { > >>>>> + compatible = "qcom,sm8650-videocc"; > >>>>> + reg = <0 0x0aaf0000 0 0x10000>; > >>>>> + clocks = <&bi_tcxo_div2>, > >>>>> + <&gcc GCC_VIDEO_AHB_CLK>; > >>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; > >>>>> + required-opps = <&rpmhpd_opp_low_svs>; > >>>> > >>>> The required-opps should no longer be necessary. > >>>> > >>> > >>> Sure, will check and remove this if not required. > >> > >> > >> I checked further on this and without required-opps, if there is no vote > >> on the power-domain & its peer from any other consumers, when runtime > >> get is called on device, it enables the power domain just at the minimum > >> non-zero level. But in some cases, the minimum non-zero level of > >> power-domain could be just retention and is not sufficient for clock > >> controller to operate, hence required-opps property is needed to specify > >> the minimum level required on power-domain for this clock controller. > > > > In which cases? If it ends up with the retention vote, it is a bug > > which must be fixed. > > > > The minimum non-zero level(configured from bootloaders) of MMCX is > retention on few chipsets but it can vary across the chipsets. Hence to > be on safer side from our end, it is good to have required-opps in DT to > specify the minimum level required for this clock controller. We are discussing sm8650, not some abstract chipset. Does it list retention or low_svs as a minimal level for MMCX? > > Thanks, > Jagadeesh > > >> > >> Thanks, > >> Jagadeesh > >> > >>> > >>>>> + #clock-cells = <1>; > >>>>> + #reset-cells = <1>; > >>>>> + #power-domain-cells = <1>; > >>>>> + }; > >>>>> + > >>>>> + camcc: clock-controller@ade0000 { > >>>>> + compatible = "qcom,sm8650-camcc"; > >>>>> + reg = <0 0x0ade0000 0 0x20000>; > >>>>> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, > >>>>> + <&bi_tcxo_div2>, > >>>>> + <&bi_tcxo_ao_div2>, > >>>>> + <&sleep_clk>; > >>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; > >>>>> + required-opps = <&rpmhpd_opp_low_svs>; > >>>>> + #clock-cells = <1>; > >>>>> + #reset-cells = <1>; > >>>>> + #power-domain-cells = <1>; > >>>>> + }; > >>>>> + > >>>>> mdss: display-subsystem@ae00000 { > >>>>> compatible = "qcom,sm8650-mdss"; > >>>>> reg = <0 0x0ae00000 0 0x1000>; > >>>>> -- > >>>>> 2.43.0 > >>>>> > >>>>> > >>>> > >>>> > > > > > >
On 4/4/2024 11:00 AM, Dmitry Baryshkov wrote: > On Thu, 4 Apr 2024 at 08:13, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 4/3/2024 9:24 PM, Dmitry Baryshkov wrote: >>> On Wed, 3 Apr 2024 at 10:16, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 3/25/2024 11:38 AM, Jagadeesh Kona wrote: >>>>> >>>>> >>>>> On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: >>>>>> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> >>>>>> wrote: >>>>>>> >>>>>>> Add device nodes for video and camera clock controllers on Qualcomm >>>>>>> SM8650 platform. >>>>>>> >>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>>>>>> --- >>>>>>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 28 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>> index 32c0a7b9aded..d862aa6be824 100644 >>>>>>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>> @@ -4,6 +4,8 @@ >>>>>>> */ >>>>>>> >>>>>>> #include <dt-bindings/clock/qcom,rpmh.h> >>>>>>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> >>>>>>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> >>>>>>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> >>>>>>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> >>>>>>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> >>>>>>> @@ -3110,6 +3112,32 @@ opp-202000000 { >>>>>>> }; >>>>>>> }; >>>>>>> >>>>>>> + videocc: clock-controller@aaf0000 { >>>>>>> + compatible = "qcom,sm8650-videocc"; >>>>>>> + reg = <0 0x0aaf0000 0 0x10000>; >>>>>>> + clocks = <&bi_tcxo_div2>, >>>>>>> + <&gcc GCC_VIDEO_AHB_CLK>; >>>>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>>>>>> + required-opps = <&rpmhpd_opp_low_svs>; >>>>>> >>>>>> The required-opps should no longer be necessary. >>>>>> >>>>> >>>>> Sure, will check and remove this if not required. >>>> >>>> >>>> I checked further on this and without required-opps, if there is no vote >>>> on the power-domain & its peer from any other consumers, when runtime >>>> get is called on device, it enables the power domain just at the minimum >>>> non-zero level. But in some cases, the minimum non-zero level of >>>> power-domain could be just retention and is not sufficient for clock >>>> controller to operate, hence required-opps property is needed to specify >>>> the minimum level required on power-domain for this clock controller. >>> >>> In which cases? If it ends up with the retention vote, it is a bug >>> which must be fixed. >>> >> >> The minimum non-zero level(configured from bootloaders) of MMCX is >> retention on few chipsets but it can vary across the chipsets. Hence to >> be on safer side from our end, it is good to have required-opps in DT to >> specify the minimum level required for this clock controller. > > We are discussing sm8650, not some abstract chipset. Does it list > retention or low_svs as a minimal level for MMCX? > Actually, the minimum level for MMCX is external to the clock controllers. But the clock controller requires MMCX to be atleast at lowsvs for it to be functional. Hence we need to keep required-opps to ensure the same without relying on the actual minimum level for MMCX. Thanks, Jagadeesh >>>>> >>>>>>> + #clock-cells = <1>; >>>>>>> + #reset-cells = <1>; >>>>>>> + #power-domain-cells = <1>; >>>>>>> + }; >>>>>>> + >>>>>>> + camcc: clock-controller@ade0000 { >>>>>>> + compatible = "qcom,sm8650-camcc"; >>>>>>> + reg = <0 0x0ade0000 0 0x20000>; >>>>>>> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, >>>>>>> + <&bi_tcxo_div2>, >>>>>>> + <&bi_tcxo_ao_div2>, >>>>>>> + <&sleep_clk>; >>>>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>>>>>> + required-opps = <&rpmhpd_opp_low_svs>; >>>>>>> + #clock-cells = <1>; >>>>>>> + #reset-cells = <1>; >>>>>>> + #power-domain-cells = <1>; >>>>>>> + }; >>>>>>> + >>>>>>> mdss: display-subsystem@ae00000 { >>>>>>> compatible = "qcom,sm8650-mdss"; >>>>>>> reg = <0 0x0ae00000 0 0x1000>; >>>>>>> -- >>>>>>> 2.43.0 >>>>>>> >>>>>>> >>>>>> >>>>>> >>> >>> >>> > > >
On Thu, 4 Apr 2024 at 13:06, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 4/4/2024 11:00 AM, Dmitry Baryshkov wrote: > > On Thu, 4 Apr 2024 at 08:13, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >> > >> > >> > >> On 4/3/2024 9:24 PM, Dmitry Baryshkov wrote: > >>> On Wed, 3 Apr 2024 at 10:16, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 3/25/2024 11:38 AM, Jagadeesh Kona wrote: > >>>>> > >>>>> > >>>>> On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: > >>>>>> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> > >>>>>> wrote: > >>>>>>> > >>>>>>> Add device nodes for video and camera clock controllers on Qualcomm > >>>>>>> SM8650 platform. > >>>>>>> > >>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > >>>>>>> --- > >>>>>>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ > >>>>>>> 1 file changed, 28 insertions(+) > >>>>>>> > >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>>>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>>>> index 32c0a7b9aded..d862aa6be824 100644 > >>>>>>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>>>> @@ -4,6 +4,8 @@ > >>>>>>> */ > >>>>>>> > >>>>>>> #include <dt-bindings/clock/qcom,rpmh.h> > >>>>>>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> > >>>>>>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> > >>>>>>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> > >>>>>>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> > >>>>>>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> > >>>>>>> @@ -3110,6 +3112,32 @@ opp-202000000 { > >>>>>>> }; > >>>>>>> }; > >>>>>>> > >>>>>>> + videocc: clock-controller@aaf0000 { > >>>>>>> + compatible = "qcom,sm8650-videocc"; > >>>>>>> + reg = <0 0x0aaf0000 0 0x10000>; > >>>>>>> + clocks = <&bi_tcxo_div2>, > >>>>>>> + <&gcc GCC_VIDEO_AHB_CLK>; > >>>>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; > >>>>>>> + required-opps = <&rpmhpd_opp_low_svs>; > >>>>>> > >>>>>> The required-opps should no longer be necessary. > >>>>>> > >>>>> > >>>>> Sure, will check and remove this if not required. > >>>> > >>>> > >>>> I checked further on this and without required-opps, if there is no vote > >>>> on the power-domain & its peer from any other consumers, when runtime > >>>> get is called on device, it enables the power domain just at the minimum > >>>> non-zero level. But in some cases, the minimum non-zero level of > >>>> power-domain could be just retention and is not sufficient for clock > >>>> controller to operate, hence required-opps property is needed to specify > >>>> the minimum level required on power-domain for this clock controller. > >>> > >>> In which cases? If it ends up with the retention vote, it is a bug > >>> which must be fixed. > >>> > >> > >> The minimum non-zero level(configured from bootloaders) of MMCX is > >> retention on few chipsets but it can vary across the chipsets. Hence to > >> be on safer side from our end, it is good to have required-opps in DT to > >> specify the minimum level required for this clock controller. > > > > We are discussing sm8650, not some abstract chipset. Does it list > > retention or low_svs as a minimal level for MMCX? > > > > Actually, the minimum level for MMCX is external to the clock > controllers. Yes, it comes from cmd-db > But the clock controller requires MMCX to be atleast at > lowsvs for it to be functional. Correct > Hence we need to keep required-opps to > ensure the same without relying on the actual minimum level for MMCX. And this is not correct. There is no need for the DT to be redundant. I plan to send patches removing the existing required-opps when they are not required.
On 4/4/2024 9:35 PM, Dmitry Baryshkov wrote: > On Thu, 4 Apr 2024 at 13:06, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 4/4/2024 11:00 AM, Dmitry Baryshkov wrote: >>> On Thu, 4 Apr 2024 at 08:13, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 4/3/2024 9:24 PM, Dmitry Baryshkov wrote: >>>>> On Wed, 3 Apr 2024 at 10:16, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 3/25/2024 11:38 AM, Jagadeesh Kona wrote: >>>>>>> >>>>>>> >>>>>>> On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: >>>>>>>> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> >>>>>>>> wrote: >>>>>>>>> >>>>>>>>> Add device nodes for video and camera clock controllers on Qualcomm >>>>>>>>> SM8650 platform. >>>>>>>>> >>>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>>>>>>>> --- >>>>>>>>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ >>>>>>>>> 1 file changed, 28 insertions(+) >>>>>>>>> >>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>>>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>>>> index 32c0a7b9aded..d862aa6be824 100644 >>>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>>>> @@ -4,6 +4,8 @@ >>>>>>>>> */ >>>>>>>>> >>>>>>>>> #include <dt-bindings/clock/qcom,rpmh.h> >>>>>>>>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> >>>>>>>>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> >>>>>>>>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> >>>>>>>>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> >>>>>>>>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> >>>>>>>>> @@ -3110,6 +3112,32 @@ opp-202000000 { >>>>>>>>> }; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> + videocc: clock-controller@aaf0000 { >>>>>>>>> + compatible = "qcom,sm8650-videocc"; >>>>>>>>> + reg = <0 0x0aaf0000 0 0x10000>; >>>>>>>>> + clocks = <&bi_tcxo_div2>, >>>>>>>>> + <&gcc GCC_VIDEO_AHB_CLK>; >>>>>>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>>>>>>>> + required-opps = <&rpmhpd_opp_low_svs>; >>>>>>>> >>>>>>>> The required-opps should no longer be necessary. >>>>>>>> >>>>>>> >>>>>>> Sure, will check and remove this if not required. >>>>>> >>>>>> >>>>>> I checked further on this and without required-opps, if there is no vote >>>>>> on the power-domain & its peer from any other consumers, when runtime >>>>>> get is called on device, it enables the power domain just at the minimum >>>>>> non-zero level. But in some cases, the minimum non-zero level of >>>>>> power-domain could be just retention and is not sufficient for clock >>>>>> controller to operate, hence required-opps property is needed to specify >>>>>> the minimum level required on power-domain for this clock controller. >>>>> >>>>> In which cases? If it ends up with the retention vote, it is a bug >>>>> which must be fixed. >>>>> >>>> >>>> The minimum non-zero level(configured from bootloaders) of MMCX is >>>> retention on few chipsets but it can vary across the chipsets. Hence to >>>> be on safer side from our end, it is good to have required-opps in DT to >>>> specify the minimum level required for this clock controller. >>> >>> We are discussing sm8650, not some abstract chipset. Does it list >>> retention or low_svs as a minimal level for MMCX? >>> >> >> Actually, the minimum level for MMCX is external to the clock >> controllers. > > Yes, it comes from cmd-db > >> But the clock controller requires MMCX to be atleast at >> lowsvs for it to be functional. > > Correct > >> Hence we need to keep required-opps to >> ensure the same without relying on the actual minimum level for MMCX. > > And this is not correct. There is no need for the DT to be redundant. > I plan to send patches removing the existing required-opps when they > are not required. > I agree this is not required if cmd-db minimum level is already at lowsvs. But since MMCX running at lowsvs is a mandatory requirement for clock controller to operate, I believe it is good to have required-opps to ensure we meet this requirement in all cases, rather than relying on the cmd-db minimum level which we have no control over. Thanks, Jagadeesh
On Fri, 5 Apr 2024 at 09:01, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > > > > On 4/4/2024 9:35 PM, Dmitry Baryshkov wrote: > > On Thu, 4 Apr 2024 at 13:06, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >> > >> > >> > >> On 4/4/2024 11:00 AM, Dmitry Baryshkov wrote: > >>> On Thu, 4 Apr 2024 at 08:13, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >>>> > >>>> > >>>> > >>>> On 4/3/2024 9:24 PM, Dmitry Baryshkov wrote: > >>>>> On Wed, 3 Apr 2024 at 10:16, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>> On 3/25/2024 11:38 AM, Jagadeesh Kona wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: > >>>>>>>> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> > >>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>> Add device nodes for video and camera clock controllers on Qualcomm > >>>>>>>>> SM8650 platform. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> > >>>>>>>>> --- > >>>>>>>>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ > >>>>>>>>> 1 file changed, 28 insertions(+) > >>>>>>>>> > >>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>>>>>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>>>>>> index 32c0a7b9aded..d862aa6be824 100644 > >>>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi > >>>>>>>>> @@ -4,6 +4,8 @@ > >>>>>>>>> */ > >>>>>>>>> > >>>>>>>>> #include <dt-bindings/clock/qcom,rpmh.h> > >>>>>>>>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> > >>>>>>>>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> > >>>>>>>>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> > >>>>>>>>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> > >>>>>>>>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> > >>>>>>>>> @@ -3110,6 +3112,32 @@ opp-202000000 { > >>>>>>>>> }; > >>>>>>>>> }; > >>>>>>>>> > >>>>>>>>> + videocc: clock-controller@aaf0000 { > >>>>>>>>> + compatible = "qcom,sm8650-videocc"; > >>>>>>>>> + reg = <0 0x0aaf0000 0 0x10000>; > >>>>>>>>> + clocks = <&bi_tcxo_div2>, > >>>>>>>>> + <&gcc GCC_VIDEO_AHB_CLK>; > >>>>>>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; > >>>>>>>>> + required-opps = <&rpmhpd_opp_low_svs>; > >>>>>>>> > >>>>>>>> The required-opps should no longer be necessary. > >>>>>>>> > >>>>>>> > >>>>>>> Sure, will check and remove this if not required. > >>>>>> > >>>>>> > >>>>>> I checked further on this and without required-opps, if there is no vote > >>>>>> on the power-domain & its peer from any other consumers, when runtime > >>>>>> get is called on device, it enables the power domain just at the minimum > >>>>>> non-zero level. But in some cases, the minimum non-zero level of > >>>>>> power-domain could be just retention and is not sufficient for clock > >>>>>> controller to operate, hence required-opps property is needed to specify > >>>>>> the minimum level required on power-domain for this clock controller. > >>>>> > >>>>> In which cases? If it ends up with the retention vote, it is a bug > >>>>> which must be fixed. > >>>>> > >>>> > >>>> The minimum non-zero level(configured from bootloaders) of MMCX is > >>>> retention on few chipsets but it can vary across the chipsets. Hence to > >>>> be on safer side from our end, it is good to have required-opps in DT to > >>>> specify the minimum level required for this clock controller. > >>> > >>> We are discussing sm8650, not some abstract chipset. Does it list > >>> retention or low_svs as a minimal level for MMCX? > >>> > >> > >> Actually, the minimum level for MMCX is external to the clock > >> controllers. > > > > Yes, it comes from cmd-db > > > >> But the clock controller requires MMCX to be atleast at > >> lowsvs for it to be functional. > > > > Correct > > > >> Hence we need to keep required-opps to > >> ensure the same without relying on the actual minimum level for MMCX. > > > > And this is not correct. There is no need for the DT to be redundant. > > I plan to send patches removing the existing required-opps when they > > are not required. > > > I agree this is not required if cmd-db minimum level is already at > lowsvs. But since MMCX running at lowsvs is a mandatory requirement for > clock controller to operate, I believe it is good to have required-opps > to ensure we meet this requirement in all cases, rather than relying on > the cmd-db minimum level which we have no control over. IIf we follow this logic, we should throw cmd-db away and hardcode all those values in the RPMh drivers. We have cmd-db. If it is correct, there is no need to duplicate it. If it is incorrect, it is a bug that should be fixed or worked around. > > Thanks, > Jagadeesh >
On 4/5/2024 1:14 PM, Dmitry Baryshkov wrote: > On Fri, 5 Apr 2024 at 09:01, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >> >> >> >> On 4/4/2024 9:35 PM, Dmitry Baryshkov wrote: >>> On Thu, 4 Apr 2024 at 13:06, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>> >>>> >>>> >>>> On 4/4/2024 11:00 AM, Dmitry Baryshkov wrote: >>>>> On Thu, 4 Apr 2024 at 08:13, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 4/3/2024 9:24 PM, Dmitry Baryshkov wrote: >>>>>>> On Wed, 3 Apr 2024 at 10:16, Jagadeesh Kona <quic_jkona@quicinc.com> wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On 3/25/2024 11:38 AM, Jagadeesh Kona wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> On 3/21/2024 6:43 PM, Dmitry Baryshkov wrote: >>>>>>>>>> On Thu, 21 Mar 2024 at 11:27, Jagadeesh Kona <quic_jkona@quicinc.com> >>>>>>>>>> wrote: >>>>>>>>>>> >>>>>>>>>>> Add device nodes for video and camera clock controllers on Qualcomm >>>>>>>>>>> SM8650 platform. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>>>>>>>>>> --- >>>>>>>>>>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ >>>>>>>>>>> 1 file changed, 28 insertions(+) >>>>>>>>>>> >>>>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>>>>>> b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>>>>>> index 32c0a7b9aded..d862aa6be824 100644 >>>>>>>>>>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>>>>>>>>>> @@ -4,6 +4,8 @@ >>>>>>>>>>> */ >>>>>>>>>>> >>>>>>>>>>> #include <dt-bindings/clock/qcom,rpmh.h> >>>>>>>>>>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> >>>>>>>>>>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> >>>>>>>>>>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> >>>>>>>>>>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> >>>>>>>>>>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> >>>>>>>>>>> @@ -3110,6 +3112,32 @@ opp-202000000 { >>>>>>>>>>> }; >>>>>>>>>>> }; >>>>>>>>>>> >>>>>>>>>>> + videocc: clock-controller@aaf0000 { >>>>>>>>>>> + compatible = "qcom,sm8650-videocc"; >>>>>>>>>>> + reg = <0 0x0aaf0000 0 0x10000>; >>>>>>>>>>> + clocks = <&bi_tcxo_div2>, >>>>>>>>>>> + <&gcc GCC_VIDEO_AHB_CLK>; >>>>>>>>>>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>>>>>>>>>> + required-opps = <&rpmhpd_opp_low_svs>; >>>>>>>>>> >>>>>>>>>> The required-opps should no longer be necessary. >>>>>>>>>> >>>>>>>>> >>>>>>>>> Sure, will check and remove this if not required. >>>>>>>> >>>>>>>> >>>>>>>> I checked further on this and without required-opps, if there is no vote >>>>>>>> on the power-domain & its peer from any other consumers, when runtime >>>>>>>> get is called on device, it enables the power domain just at the minimum >>>>>>>> non-zero level. But in some cases, the minimum non-zero level of >>>>>>>> power-domain could be just retention and is not sufficient for clock >>>>>>>> controller to operate, hence required-opps property is needed to specify >>>>>>>> the minimum level required on power-domain for this clock controller. >>>>>>> >>>>>>> In which cases? If it ends up with the retention vote, it is a bug >>>>>>> which must be fixed. >>>>>>> >>>>>> >>>>>> The minimum non-zero level(configured from bootloaders) of MMCX is >>>>>> retention on few chipsets but it can vary across the chipsets. Hence to >>>>>> be on safer side from our end, it is good to have required-opps in DT to >>>>>> specify the minimum level required for this clock controller. >>>>> >>>>> We are discussing sm8650, not some abstract chipset. Does it list >>>>> retention or low_svs as a minimal level for MMCX? >>>>> >>>> >>>> Actually, the minimum level for MMCX is external to the clock >>>> controllers. >>> >>> Yes, it comes from cmd-db >>> >>>> But the clock controller requires MMCX to be atleast at >>>> lowsvs for it to be functional. >>> >>> Correct >>> >>>> Hence we need to keep required-opps to >>>> ensure the same without relying on the actual minimum level for MMCX. >>> >>> And this is not correct. There is no need for the DT to be redundant. >>> I plan to send patches removing the existing required-opps when they >>> are not required. >>> In my opinion, it is better not to remove the required-opps for the existing targets atleast since it may lead to some random clock issues if cmd-db minimum level is lower than the HW recommended voltage level clock controller requires. >> I agree this is not required if cmd-db minimum level is already at >> lowsvs. But since MMCX running at lowsvs is a mandatory requirement for >> clock controller to operate, I believe it is good to have required-opps >> to ensure we meet this requirement in all cases, rather than relying on >> the cmd-db minimum level which we have no control over. > > IIf we follow this logic, we should throw cmd-db away and hardcode all > those values in the RPMh drivers. > > We have cmd-db. If it is correct, there is no need to duplicate it. If > it is incorrect, it is a bug that should be fixed or worked around. > Sure will check and remove required-opps property for SM8650. Thanks, Jagadeesh
Hi Konrad, On 3/23/24 02:33, Konrad Dybcio wrote: > On 21.03.2024 14:07, Vladimir Zapolskiy wrote: >> Hello Jagadeesh, >> >> On 3/21/24 11:25, Jagadeesh Kona wrote: >>> Add device nodes for video and camera clock controllers on Qualcomm >>> SM8650 platform. >>> >>> Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> >>> --- >>> arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ >>> 1 file changed, 28 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>> index 32c0a7b9aded..d862aa6be824 100644 >>> --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi >>> +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi >>> @@ -4,6 +4,8 @@ >>> */ >>> #include <dt-bindings/clock/qcom,rpmh.h> >>> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> >>> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> >>> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> >>> #include <dt-bindings/clock/qcom,sm8650-gcc.h> >>> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> >>> @@ -3110,6 +3112,32 @@ opp-202000000 { >>> }; >>> }; >>> + videocc: clock-controller@aaf0000 { >>> + compatible = "qcom,sm8650-videocc"; >>> + reg = <0 0x0aaf0000 0 0x10000>; >>> + clocks = <&bi_tcxo_div2>, >>> + <&gcc GCC_VIDEO_AHB_CLK>; >>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>> + required-opps = <&rpmhpd_opp_low_svs>; >> >> Please add default status = "disabled"; >> >>> + #clock-cells = <1>; >>> + #reset-cells = <1>; >>> + #power-domain-cells = <1>; >>> + }; >>> + >>> + camcc: clock-controller@ade0000 { >>> + compatible = "qcom,sm8650-camcc"; >>> + reg = <0 0x0ade0000 0 0x20000>; >>> + clocks = <&gcc GCC_CAMERA_AHB_CLK>, >>> + <&bi_tcxo_div2>, >>> + <&bi_tcxo_ao_div2>, >>> + <&sleep_clk>; >>> + power-domains = <&rpmhpd RPMHPD_MMCX>; >>> + required-opps = <&rpmhpd_opp_low_svs>; >> >> Please add default status = "disabled"; >> >>> + #clock-cells = <1>; >>> + #reset-cells = <1>; >>> + #power-domain-cells = <1>; >>> + }; >>> + >>> mdss: display-subsystem@ae00000 { >>> compatible = "qcom,sm8650-mdss"; >>> reg = <0 0x0ae00000 0 0x1000>; >> >> After disabling the clock controllers > > Clock controllers should never be disabled period, that defeats the > entire point of having unused clk/pd cleanup. hm, that's very sane, I didn't think about it from this point, thanks! > The only reason for them to be disabled is for cases where platform > crashes on access due to stinky "security" settings (like with audio > clocks), or when people are too lazy to upstream panel drivers and > end up partially upstreaming display-related changes and continue > using the bootloader-initialized framebuffer. This takes away from > the very little determinism we have. > -- Best wishes, Vladimir
diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi index 32c0a7b9aded..d862aa6be824 100644 --- a/arch/arm64/boot/dts/qcom/sm8650.dtsi +++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi @@ -4,6 +4,8 @@ */ #include <dt-bindings/clock/qcom,rpmh.h> +#include <dt-bindings/clock/qcom,sm8450-videocc.h> +#include <dt-bindings/clock/qcom,sm8650-camcc.h> #include <dt-bindings/clock/qcom,sm8650-dispcc.h> #include <dt-bindings/clock/qcom,sm8650-gcc.h> #include <dt-bindings/clock/qcom,sm8650-gpucc.h> @@ -3110,6 +3112,32 @@ opp-202000000 { }; }; + videocc: clock-controller@aaf0000 { + compatible = "qcom,sm8650-videocc"; + reg = <0 0x0aaf0000 0 0x10000>; + clocks = <&bi_tcxo_div2>, + <&gcc GCC_VIDEO_AHB_CLK>; + power-domains = <&rpmhpd RPMHPD_MMCX>; + required-opps = <&rpmhpd_opp_low_svs>; + #clock-cells = <1>; + #reset-cells = <1>; + #power-domain-cells = <1>; + }; + + camcc: clock-controller@ade0000 { + compatible = "qcom,sm8650-camcc"; + reg = <0 0x0ade0000 0 0x20000>; + clocks = <&gcc GCC_CAMERA_AHB_CLK>, + <&bi_tcxo_div2>, + <&bi_tcxo_ao_div2>, + <&sleep_clk>; + power-domains = <&rpmhpd RPMHPD_MMCX>; + required-opps = <&rpmhpd_opp_low_svs>; + #clock-cells = <1>; + #reset-cells = <1>; + #power-domain-cells = <1>; + }; + mdss: display-subsystem@ae00000 { compatible = "qcom,sm8650-mdss"; reg = <0 0x0ae00000 0 0x1000>;
Add device nodes for video and camera clock controllers on Qualcomm SM8650 platform. Signed-off-by: Jagadeesh Kona <quic_jkona@quicinc.com> --- arch/arm64/boot/dts/qcom/sm8650.dtsi | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)