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

[H. Nikolaus Schaller]

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.

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