On Mon, Jun 12, 2023 at 04:33:09PM +0300, Dmitry Baryshkov wrote:
> On 12/06/2023 12:01, Stephan Gerhold wrote:
> > On Mon, Jun 12, 2023 at 08:39:19AM +0300, Dmitry Baryshkov wrote:
> > > APQ8064 has 4 speed bins, each of them having from 4 to 6 categorization
> > > kinds. Provide tables necessary to handle voltage scaling on this SoC.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > > ---
> > > arch/arm/boot/dts/qcom-apq8064.dtsi | 1017 +++++++++++++++++++++++++++
> > > 1 file changed, 1017 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/qcom-apq8064.dtsi b/arch/arm/boot/dts/qcom-apq8064.dtsi
> > > index 4ef13f3d702b..f35853b59544 100644
> > > --- a/arch/arm/boot/dts/qcom-apq8064.dtsi
> > > +++ b/arch/arm/boot/dts/qcom-apq8064.dtsi
> > > @@ -49,6 +49,9 @@ CPU0: cpu@0 {
> > > clocks = <&kraitcc KRAIT_CPU_0>;
> > > clock-names = "cpu";
> > > clock-latency = <100000>;
> > > + vdd-mem-supply = <&pm8921_l24>;
> > > + vdd-dig-supply = <&pm8921_s3>;
> > > + vdd-core-supply = <&saw0_vreg>;
> > > interconnects = <&kraitcc MASTER_KRAIT_L2 &kraitcc SLAVE_KRAIT_L2>;
> > > operating-points-v2 = <&cpu_opp_table>;
> > > #cooling-cells = <2>;
> > > @@ -66,6 +69,9 @@ CPU1: cpu@1 {
> > > clocks = <&kraitcc KRAIT_CPU_1>;
> > > clock-names = "cpu";
> > > clock-latency = <100000>;
> > > + vdd-mem-supply = <&pm8921_l24>;
> > > + vdd-dig-supply = <&pm8921_s3>;
> > > + vdd-core-supply = <&saw1_vreg>;
> > > interconnects = <&kraitcc MASTER_KRAIT_L2 &kraitcc SLAVE_KRAIT_L2>;
> > > operating-points-v2 = <&cpu_opp_table>;
> > > #cooling-cells = <2>;
> > > @@ -83,6 +89,9 @@ CPU2: cpu@2 {
> > > clocks = <&kraitcc KRAIT_CPU_2>;
> > > clock-names = "cpu";
> > > clock-latency = <100000>;
> > > + vdd-mem-supply = <&pm8921_l24>;
> > > + vdd-dig-supply = <&pm8921_s3>;
> > > + vdd-core-supply = <&saw2_vreg>;
> > > interconnects = <&kraitcc MASTER_KRAIT_L2 &kraitcc SLAVE_KRAIT_L2>;
> > > operating-points-v2 = <&cpu_opp_table>;
> > > #cooling-cells = <2>;
> > > @@ -100,6 +109,9 @@ CPU3: cpu@3 {
> > > clocks = <&kraitcc KRAIT_CPU_3>;
> > > clock-names = "cpu";
> > > clock-latency = <100000>;
> > > + vdd-mem-supply = <&pm8921_l24>;
> > > + vdd-dig-supply = <&pm8921_s3>;
> > > + vdd-core-supply = <&saw3_vreg>;
> > > interconnects = <&kraitcc MASTER_KRAIT_L2 &kraitcc SLAVE_KRAIT_L2>;
> > > operating-points-v2 = <&cpu_opp_table>;
> > > #cooling-cells = <2>;
> > > @@ -132,6 +144,81 @@ cpu_opp_table: opp-table-cpu {
> > > opp-384000000 {
> > > opp-hz = /bits/ 64 <384000000>;
> > > opp-peak-kBps = <384000>;
> > > + opp-microvolt-speed0-pvs0 = <1050000 1050000 1150000>,
> > > + <950000 950000 1150000>,
> > > + <950000 950000 975000>;
> >
> > I think this won't result in the correct switch order without making
> > some changes to the OPP core. In _set_opp() the OPP core does
> >
> > /* Scaling up? Configure required OPPs before frequency */
> > if (!scaling_down) {
> > _set_required_opps();
> > _set_opp_bw();
> > opp_table->config_regulators();
> > }
> >
> > opp_table->config_clks();
> >
> > /* Scaling down? Configure required OPPs after frequency */
> > if (scaling_down) {
> > opp_table->config_regulators();
> > _set_opp_bw();
> > _set_required_opps();
> > }
> >
> > Since the "bandwidth" for the L2 cache is set before the regulators
> > there is a short window where the L2 clock is running at a high
> > frequency with too low voltage, which could potentially cause
> > instability. On downstream this seems to be done in the proper order [1].
> >
> > I'm not sure if the order in the OPP core is on purpose. If not, you
> > could propose moving the config_regulators() first (for scaling up)
> > and last (for scaling down). This would resolve the problem.
>
> Nice catch, I missed this ordering point.
>
> >
> > The alternative that I've already argued for on IRC in #linux-msm a
> > couple of days ago would be to give the L2 cache (here: "interconnect")
> > an own OPP table where it can describe its voltage requirements,
> > independent from the CPU. That way the icc_set_bw() would be guaranteed
> > to apply the correct voltage before adjusting the L2 cache clock. It
> > looks like the "l2_level" voltages for vdd_dig and vdd_mem are not
> > speedbin/PVS-specific [2] so this would also significantly reduce the DT
> > size, since you wouldn't need to repeat the same vdd_dig/vdd_mem
> > voltages for all of them.
>
> Yes. I fact our discussion triggered me to do this patchset.
>
> So, another option would be to have something like the following snippet. Do
> you know if we are allowed to squish additional data into the L2 cache DT
> node?
>
I have a similar implementation with the l2 devfreq driver where I need
to put a compatible in the l2-cache node. From what I observed, keeping
the l2-cache node in the cpus node makes the extra compile not work
(nothing is probed) but moving the l2-cache node in the soc node and
referencing the phandle makes the compatible correctly works and that
doesn't seems to cause any problem. IMHO it would be better to have a
separate opp table for l2, should keep things more organized.
> CPU0: cpu@0 {
> vdd-core-supply = <&saw0_vreg>;
> interconnects = <&L2 MASTER_KRAIT_L2 &L2 SLAVE_KRAIT_L2>;
> operating-points-v2 = <&cpu_opp_table>;
> };
>
> L2: l2-cache {
> compatible = "qcom,apq8064-l2-cache", "cache";
>
> clocks = <&kraitcc KRAIT_L2>;
> vdd-mem-supply = <&pm8921_l24>;
> vdd-dig-supply = <&pm8921_s3>;
> operating-points-v2 = <&l2_opp_table>;
>
> l2_opp_table {
> compatible = "operating-points-v2";
> opp-384000000 {
> opp-hz = /bits/ 64 <384000000>;
> opp-microvolt = <1050000 1050000 1150000>,
> <950000 950000 1150000>;
> };
>
> opp-648000000 {
> opp-hz = /bits/ 64 <648000000>;
> opp-microvolt = <1050000 1050000 1150000>,
> <1050000 1050000 1150000>;
> };
>
> opp-1134000000 {
> opp-hz = /bits/ 64 <1134000000>;
> opp-microvolt = <1150000 1150000 1150000>,
> <1150000 1150000 1150000>;
> };
> };
> };
>
> >
> > Thanks,
> > Stephan
> >
> > [1]: https://git.codelinaro.org/clo/la/kernel/msm/-/blob/LA.AF.1.2.1-08410-8064.0/arch/arm/mach-msm/acpuclock-krait.c#L529-588
> > [2]: https://git.codelinaro.org/clo/la/kernel/msm/-/blob/LA.AF.1.2.1-08410-8064.0/arch/arm/mach-msm/acpuclock-8064.c#L118-135
>
> --
> With best wishes
> Dmitry
>
--
Ansuel
