Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH v25 0/7] soc: mediatek: SVS: introduce MTK SVS
To:     Kevin Hilman <khilman@kernel.org>,
        Chen-Yu Tsai <wenst@chromium.org>
Cc:     Roger Lu <roger.lu@mediatek.com>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        Enric Balletbo Serra <eballetbo@gmail.com>,
        Rob Herring <robh+dt@kernel.org>,
        Nicolas Boichat <drinkcat@google.com>,
        Stephen Boyd <sboyd@kernel.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Fan Chen <fan.chen@mediatek.com>,
        Charles Yang <Charles.Yang@mediatek.com>,
        Angus Lin <Angus.Lin@mediatek.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Nishanth Menon <nm@ti.com>, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-pm@vger.kernel.org,
        Project_Global_Chrome_Upstream_Group@mediatek.com,
        Guenter Roeck <linux@roeck-us.net>,
        Jia-wei Chang <jia-wei.chang@mediatek.com>,
        =?UTF-8?B?UmV4LUJDIENoZW4gKOmZs+afj+i+sCk=?= 
        <rex-bc.chen@mediatek.com>
From:   Chanwoo Choi <cw00.choi@samsung.com>
Organization: Samsung Electronics
Message-ID: <83b10ab5-147a-dec7-66d9-9e23f67a5777@samsung.com>
Date:   Tue, 31 May 2022 14:55:24 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101
        Thunderbird/59.0
MIME-Version: 1.0
In-Reply-To: <7hleup9sh8.fsf@baylibre.com>
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20220516004311.18358-1-roger.lu@mediatek.com>
        <CAGXv+5GSdWPZe3fNpBJ_WW0zCL8Skg6fHx9ATxaKU1hyMEt2Ww@mail.gmail.com>
        <7h4k1ndaui.fsf@baylibre.com> <7hy1yzbtb7.fsf@baylibre.com>
        <CAGXv+5GT=3m=pVPwUOWR42BR=emCpBXvvoAiRV7YKt2kEKWdAQ@mail.gmail.com>
        <CGME20220519182512epcas1p3020bd4713580c9244f759971b8bd2c3a@epcas1p3.samsung.com>
        <7hmtfdbcsc.fsf@baylibre.com>
        <5a1767dc-ba2d-4de5-d8fe-2f308d3318a9@samsung.com>
        <CAGXv+5EsgiXCpe-8H0cQ=qm_Nq+yfM_a4b1L=hOFP6mcwfZymw@mail.gmail.com>
        <eacf7a67-bc64-a764-e23d-c733a4666b8a@samsung.com>
        <CAGXv+5HEG1VJ89LDHJmMvPqafcdb2cuJj2wxv44ozPxDeHj8ig@mail.gmail.com>
        <7hleup9sh8.fsf@baylibre.com>
Precedence: bulk

On 5/26/22 7:07 AM, Kevin Hilman wrote:
> Chen-Yu Tsai <wenst@chromium.org> writes:
> 
>> On Fri, May 20, 2022 at 5:53 PM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>
>>> Hi Kevin,
>>>
>>> On 5/20/22 11:42 AM, Chen-Yu Tsai wrote:
>>>> On Fri, May 20, 2022 at 9:28 AM Chanwoo Choi <cw00.choi@samsung.com> wrote:
>>>>>
>>>>> Hi Kevin, Chen-Yu,
>>>>>
>>>>> On 5/20/22 3:25 AM, Kevin Hilman wrote:
>>>>>> Chen-Yu Tsai <wenst@chromium.org> writes:
>>>>>>
>>>>>>> n Wed, May 18, 2022 at 8:03 AM Kevin Hilman <khilman@kernel.org> wrote:
>>>>>>>>
>>>>>>>> Kevin Hilman <khilman@kernel.org> writes:
>>>>>>>>
>>>>>>>>> Chen-Yu Tsai <wenst@chromium.org> writes:
>>>>>>>>>
>>>>>>>>>> On Mon, May 16, 2022 at 8:43 AM Roger Lu <roger.lu@mediatek.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> The Smart Voltage Scaling(SVS) engine is a piece of hardware
>>>>>>>>>>> which calculates suitable SVS bank voltages to OPP voltage table.
>>>>>>>>>>> Then, DVFS driver could apply those SVS bank voltages to PMIC/Buck
>>>>>>>>>>> when receiving OPP_EVENT_ADJUST_VOLTAGE.
>>>>>>>>>>>
>>>>>>>>>>> 1. SVS driver uses OPP adjust event in [1] to update OPP table voltage part.
>>>>>>>>>>> 2. SVS driver gets thermal/GPU device by node [2][3] and CPU device by get_cpu_device().
>>>>>>>>>>> After retrieving subsys device, SVS driver calls device_link_add() to make sure probe/suspend callback priority.
>>>>>>>>>>>
>>>>>>>>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/commit/?h=opp/linux-next&id=25cb20a212a1f989385dfe23230817e69c62bee5
>>>>>>>>>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/vireshk/pm.git/commit/?h=opp/linux-next&id=b325ce39785b1408040d90365a6ab1aa36e94f87
>>>>>>>>>>> [3] https://git.kernel.org/pub/scm/linux/kernel/git/matthias.bgg/linux.git/commit/?h=v5.16-next/dts64&id=a8168cebf1bca1b5269e8a7eb2626fb76814d6e2
>>>>>>>>>>>
>>>>>>>>>>> Change since v24:
>>>>>>>>>>> - Rebase to Linux 5.18-rc6
>>>>>>>>>>> - Show specific fail log in svs_platform_probe() to help catch which step fails quickly
>>>>>>>>>>> - Remove struct svs_bank member "pd_dev" because all subsys device's power domain has been merged into one node like above [3]
>>>>>>>>>>>
>>>>>>>>>>> Test in below environment:
>>>>>>>>>>> SW: Integration Tree [4] + Thermal patch [5] + SVS v25 (this patchset)
>>>>>>>>>>> HW: mt8183-Krane
>>>>>>>>>>>
>>>>>>>>>>> [4] https://protect2.fireeye.com/v1/url?k=847bae75-e5f0bb43-847a253a-000babff9b5d-0b6f42041b9dea1d&q=1&e=37a26c43-8564-4808-9701-dc76d1ebbb27&u=https%3A%2F%2Fgithub.com%2Fwens%2Flinux%2Fcommits%2Fmt8183-cpufreq-cci-svs-test
>>>>>>>>>>
>>>>>>>>>> I've updated my branch to include all the latest versions of the relevant
>>>>>>>>>> patch series:
>>>>>>>>>>
>>>>>>>>>> - anx7625 DPI bus type series v2 (so the display works)
>>>>>>>>>> - MT8183 thermal series v9 (this seems to have been overlooked by the
>>>>>>>>>> maintainer)
>>>>>>>>>> - MTK SVS driver series v25
>>>>>>>>>> - devfreq: cpu based scaling support to passive governor series v5
>>>>>>>>>> - MTK CCI devfreq series v4
>>>>>>>>>> - MT8183 cpufreq series v7
>>>>>>>>>> - Additional WIP patches for panfrost MTK devfreq
>>>>>>>>>
>>>>>>>>> Thanks for preparing an integration branch Chen-Yu.
>>>>>>>>>
>>>>>>>>> I'm testing this on mt8183-pumpkin with one patch to add the CCI
>>>>>>>>> regulator[1], and the defconfig you posted in a previous rev of this
>>>>>>>>> series, but the CCI driver still causes a fault on boot[2] on my
>>>>>>>>> platform.
>>>>>>>>>
>>>>>>>>> I mentioned in earlier reviews that I think there's potentially a race
>>>>>>>>> between CCI and SVS loading since they are co-dependent.  My hunch is
>>>>>>>>> that this is still not being handled properly.
>>>>>>>>
>>>>>>>> Ah, actually it's crashing when I try to boot the platform with
>>>>>>>> `maxcpus=4` on the cmdline (which I have to do because mt8183-pumpkin is
>>>>>>>> unstable upstream with the 2nd cluster enabled.)
>>>>>
>>>>> This warning message is printed by 'WARN_ON(cpufreq_passive_unregister_notifier(devfreq))'
>>>>> on devfreq passive governor.
>>>>>
>>>>> If the cpufreq drivers are not probed before of probing cci devfreq driver
>>>>> with passive governor, passive governor shows this warning message.
>>>>> Because passive governor with CPUFREQ_PARENT_DEV depends on the cpufreq driver
>>>>> in order to get 'struct cpufreq_policy'[2].
>>>>>
>>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/tree/drivers/devfreq/governor_passive.c?h=devfreq-testing#n339
>>>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/linux.git/tree/drivers/devfreq/governor_passive.c?h=devfreq-testing#n282
>>>>>
>>>>> But, as I knew, this message might not stop the kernel. Just show the warning
>>>>> message and then return -EPROBE_DEFER error. It means that maybe try to
>>>>> probe the cci devfreq driver on late time of kernel booting
>>>>> and then will be working. But, I need the full kernel booting log
>>>>> and the booting sequence of between cpufreq and cci devfreq driver.
>>>>
>>>> Maybe just use a standard dev_warn() instead? WARN_ON causes all sorts
>>>> of panicking in developers' minds. :p
>>>>
>>>>> In order to fix your issue, could you share the full booting log?
>>>>> And if possible, please explain the more detailed something about this.
>>>>
>>>> The shortened version is that on an 8 core system, with maxcpus=4,
>>>> only the first four cores are booted and have cpufreq associated.
>>>> I've not actually used this mechanism, so I don't really know what
>>>> happens if the other cores are brought up later with hotplug. Is
>>>> cpufreq expected to attach to them?
>>>>
>>>> Maybe Kevin can add some more details.
>>>>
>>>>
>>>> ChenYu
>>>>
>>>>
>>>>>>>>
>>>>>>>> The CCI driver should be a bit more robust about detecting
>>>>>>>> available/online CPUs
>>>>>>>
>>>>>>> This all seems to be handled in the devfreq passive governor.
>>>>>>
>>>>>> Well, that's the initial crash.  But the SVS driver will also go through
>>>>>> its svs_mt8183_banks[] array (including both big & little clusters) and
>>>>>> try to init SVS, so presumably that will have some problems also if only
>>>>>> one cluster is enabled.
>>>>>>
>>>>>>> And presumably we'd like to have CCI devfreq running even if just one
>>>>>>> core was booted.
>>>>>>
>>>>>> Yes, I assume so also.
>>>>>>
>>>>>>> Added Chanwoo for more ideas.
>>>>>>
>>>>>> OK, thanks.
>>>>>>
>>>>>> Kevin
>>>
>>>
>>> I tested the passive governor with my temporary test code
>>> on odroid-xu3 which contains the big.LITTLE cluster (Octa-core).
>>>
>>>
>>> [Sequence of cpufreq/devfreq driver]
>>> 1. Turn on all cpus
>>> 2. Probed cpufreq driver
>>> 3. Probed devfreq driver using passive governor with CPUFREQ_PARENT_DEV
>>>
>>> In my test case, there are no warning message during kernel booting.
>>> Also when scaling the cpu frequency of cpus of big.LITTLE clusters,
>>> temporary devfreq driver receives the notfication and then
>>> calculate the target frequency of devfreq device by iterating online cpu.
>>>
>>> If there are any h/w constraints on your case, please let me know.
>>
>> Could you run your system with maxcpus=4 added to your cmdline?
>> This is what Kevin was running.
>>
>> The current result is that the latter four cores aren't booted, so no
>> cpufreq tied to them, and the passive governor will fail to get their
>> cpufreq_policy. As mentioned before, the code path used to have a
>> WARN_ON(). Now it's a dev_warn(). It will still fail initialization
>> though.
>>
>> We're wondering if devfreq passive governor should be made to work
>> even if not all cpu cores are available when it probes.
> 
> For info, here is a boot log[1] from mt8183-pumpkin board where I'm
> testing Chen-Yu's lastest integration branch.  
> 
> As Chen-Yu said, the part that makes it trigger the warn is disabling
> some of the CPUs *at boot time*.  In this case, I'm passing `maxcpus=4`
> on the kernel command line.
> 
> Kevin
> 
> [1] https://protect2.fireeye.com/v1/url?k=05bb8eea-64309bc5-05ba05a5-74fe485cbfe7-9281bdbd13e5cf90&q=1&e=8ab47ff1-daee-4db3-a26d-6fc652568a44&u=https%3A%2F%2Ftermbin.com%2Fzidi
> 
> 

When using 'maxcpus=' on my test board, I got the warning message.
I'm fixing it and then send the patch. Thanks for the test.

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics