Re: [Letux-kernel] [RFC PATCH 0/3] Enable 1GHz support on omap36xx

[Adam Ford]

On Mon, Sep 9, 2019 at 9:57 AM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
>
> Hi Adam,
>
> > Am 09.09.2019 um 16:26 schrieb Adam Ford <aford173@xxxxxxxxx>:
> >
> > On Sat, Sep 7, 2019 at 2:38 AM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
> >>
> >> Hi Adam,
> >>
> >>> Am 02.09.2019 um 23:10 schrieb Adam Ford <aford173@xxxxxxxxx>:
> >>>
> >>> On Mon, Sep 2, 2019 at 10:46 AM H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> wrote:
> >>>>
> >>>>
> >>>>
> >>>> But my tests show that decoding works now. So you already might give it a try.
> >>>
> >>> I am traveling all this week, but I have an omap3530, DM3730
> >>> (omap3630), and an AM3517 that I use for testing.
> >>
> >> now as the omap3430 and omap3630 opp-v2 tables are installed,
> >> we could add am35x7 as well.
> >>
> >> What needs to be done:
> >>
> >> 1. add OPP-v2 table to am3517.dtsi
> >>
> >> for example copy skeleton from omap36xx.dtsi
> >>
> >> and define reasonable clock speeds. I would think about
> >> 150 MHz, 300 MHz, 600MHz.
> >
> > This might be more of a question for TI, but  can we match the 3430
> > list of frequencies?
> >
> > Something like:
> >    125000  1200000
> >    250000  1200000
> >    500000  1200000
> >    550000  1200000
> >    600000  1200000
>
> And another question: is it more derived from omap3430 or omap3630?
>
> >
> >
> >>
> >> Debatable is if we need a clock-latency definition.
> >>
> >> 2. change all voltages to 1.2V
> >>
> >>                        opp-microvolt = <1200000 1200000 1200000>;
> >>
> >> There is no point to specify 3 voltages <target min max> here since we
> >> will never need a min and a max value.
> >>
> >>                        opp-microvolt = <1200000>;
> >>
> >> should also be ok (AFAIK, parser handles single-value records).
> >>
> >> 3. AFAIK there is no speed binned eFuse...
> >>
> >> But the ti-cpufreq driver always wants to read some eFuse register.
> >>
> >> So please check if you can read 0x4800244C and 0x4830A204
> >> like on omap36xx and if they produce stable values (and not
> >> random noise).
> >
> > For the AM3517,
> >
> > 0x4800244C = 0000 0cc0
>
> If it behaves like an dm3730 (Table 1-6) this would be read as 800/600MHz
> and some reserved code in bit 7:6.
>
> If it behaves like an omap3530 (Table 1-6) this would bean 600MHz but reserved
> value for IVA Frequency.
>
>
> > 0x4830A204 = 1b86 802f
>
> would be decoded (Table 1-7) as "AM/DM37x ES1.1"
>
> omap35xx would have a different code (Table 1-9). Most similar is "OMAP35x ES2.0" with 0x1B7A E02F
>
> So this seems to answer that the am3517 is indeed a derivative of the am/dm37xx.
>
> Therefore the only OPPs would be OPP50 (300 MHz) and OPP100 (600 MHz).
>
> Only tests or TI internal documentations could show if the am3517 still
> runs stable at newly invented "OPP25" (150MHz).
>
> >
> > The AM3517 shows these are valid addresses and I read them multiple
> > times and they yielded the same results even after power cycling.
> >
> >
> >>
> >> If yes, we simply assume that am3517 is similar enough to omap3630,
> >> ignore that there is no eFuse, but read the register anyways and
> >> then ignore the bit if it is 0 or 1.
> >>
> >> This means that all OPPs can get
> >>
> >>                        opp-supported-hw = <0xffffffff 0xffffffff>;
> >>
> >> There could also be a default handler if this property is missing,
> >> but I have not researched this.
> >>
> > Like this?
> >
> > opp1-125000000 {
> >     opp-hz = /bits/ 64 <125000000>;
> >     opp-microvolt = <1200000>;
> >     opp-supported-hw = <0xffffffff 0xffffffff>;
> > };
> >
> > opp2-250000000 {
> >     opp-hz = /bits/ 64 <250000000>;
> >     opp-microvolt = <1200000>;
> >    opp-supported-hw = <0xffffffff 0xffffffff>;
> >     opp-suspend;
> > };
> >
> > opp3-500000000 {
> >     opp-hz = /bits/ 64 <500000000>;
> >     opp-microvolt = <1200000>;
> >     opp-supported-hw = <0xffffffff 0xffffffff>;
> > };
> >
> > opp4-550000000 {
> >     opp-hz = /bits/ 64 <550000000>;
> >     opp-microvolt = <1200000>;
> >     opp-supported-hw = <0xffffffff 0xffffffff>;
> > };
> >
> > opp5-600000000 {
> >     opp-hz = /bits/ 64 <600000000>;
> >     opp-microvolt = <1200000>;
> >     opp-supported-hw = <0xffffffff 0xffffffff>;
> > };
>
> Yes.
>
> >
> > What does opp-suspend do?  I noticed it in the 34xx.dtsi
>
> Good question. I think it is the OPP to be chosen before suspend:
>
> https://www.kernel.org/doc/Documentation/devicetree/bindings/opp/opp.txt
> says
>
> - opp-suspend: Marks the OPP to be used during device suspend. Only one OPP in
>   the table should have this.
>
> But that doesn't mean the drivers make use of this marker.
>
> This makes me also wonder if we should tag the OPP1G and OPP6 as "turbo-mode"...
>
> Another question that came up by private mail from Andrà was if we
> should better disable the turbo OPPs of omap34xx and 36xx by default
> (status = "disabled";) because there are concerns about overheating
> the chips and we have no thermal regulation like for omap4 & 5.
>
> But this would mean that every board DTS would have to set it explicitly
> to "enabled".
>
> And another concern is if the 1GHz OPP doesn't also need to switch the
> ABB bias LDO to a different mode. This is not done by the ti-cpufreq driver.
> Maybe it is done by some driver in mach-omap but I have not searched for.

ti-omap5-opp-supply.txt shows

Required Properties for Device Node:
- vdd-supply: phandle to regulator controlling VDD supply
- vbb-supply: phandle to regulator controlling Body Bias supply
      (Usually Adaptive Body Bias regulator)

Looking at the ti-cpufreq.c driver, it appears as if there is both a
vdd and a vbb listed in the table (line 302).  AFAICT, we should be
able to just set it up pointing VDD to the PMIC and VBB at the ABB
unless I am missing something.

Currently, I have the follwing in my device tree:

/* Reroute power feeding the CPU to come from the external PMIC */
&reg_arm
{
     vin-supply = <&sw1a_reg>;
};

&reg_soc
{
      vin-supply = <&sw1c_reg>;
};

Is this still correct with the new cpufreq driver?   I am wondering if
we need VDD and VBB references under the cpu entry.  The
ti-omap5-opp-supply ready me also reads:

/* Device Node (CPU)  */
cpus {
     cpu0: cpu@0 {
          device_type = "cpu";

...

          vdd-supply = <&vcc>;
          vbb-supply = <&abb_mpu>;
     };
};

This isn't in the ti-cpufreq.txt, however it might actually be
utilized based on a quick skim of the driver.

adam
>
> So the concern is that we will run the turbo modes outside of the TI specs
> while before applying the patch set this would be a lesser problem (OPP130
> should also be thermally limited to 90ÂC).
>
> I.e. users of 1GHz capable boards will not only see 25% more speed but
> suddenly higher SoC temperatures than the years before.
>
> >
> >> 4. add compatible to ti-cpufreq
> >> and share the register offsets, bit masks etc. with omap3630:
> >>
> >>        { .compatible = "ti,am33xx", .data = &am3x_soc_data, },
> >>        { .compatible = "ti,am3517", .data = &omap36xx_soc_data, },
> >>        { .compatible = "ti,am43", .data = &am4x_soc_data, },
> >>        { .compatible = "ti,dra7", .data = &dra7_soc_data },
> >>        { .compatible = "ti,omap3430", .data = &omap34xx_soc_data, },
> >>        { .compatible = "ti,omap3630", .data = &omap36xx_soc_data, },
> >>
> >> 5. configure for CONFIG_ARM_TI_CPUFREQ=y
> >>
> >> This should IMHO suffice.
> >
> > If you're OK with what I am proposing, I'll run some tests and submit
> > a patch.  I won't promise to fully understand what's happening.  :-)
>
> Same for me :)
>
> BR and thanks,
> Nikolaus
>