Date:   Fri, 20 May 2022 10:42:51 +0800
From:   Chen-Yu Tsai <wenst@...omium.org>
To:     Chanwoo Choi <cw00.choi@...sung.com>
Cc:     Kevin Hilman <khilman@...nel.org>,
        Roger Lu <roger.lu@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Enric Balletbo Serra <eballetbo@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Nicolas Boichat <drinkcat@...gle.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Fan Chen <fan.chen@...iatek.com>,
        Charles Yang <Charles.Yang@...iatek.com>,
        Angus Lin <Angus.Lin@...iatek.com>,
        Mark Rutland <mark.rutland@....com>,
        Nishanth Menon <nm@...com>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        Guenter Roeck <linux@...ck-us.net>,
        Jia-wei Chang <jia-wei.chang@...iatek.com>,
        Rex-BC Chen (陳柏辰) 
        <rex-bc.chen@...iatek.com>
Subject: Re: [PATCH v25 0/7] soc: mediatek: SVS: introduce MTK SVS

On Fri, May 20, 2022 at 9:28 AM Chanwoo Choi <cw00.choi@...sung.com> wrote:
>
> Hi Kevin, Chen-Yu,
>
> On 5/20/22 3:25 AM, Kevin Hilman wrote:
> > Chen-Yu Tsai <wenst@...omium.org> writes:
> >
> >> n Wed, May 18, 2022 at 8:03 AM Kevin Hilman <khilman@...nel.org> wrote:
> >>>
> >>> Kevin Hilman <khilman@...nel.org> writes:
> >>>
> >>>> Chen-Yu Tsai <wenst@...omium.org> writes:
> >>>>
> >>>>> On Mon, May 16, 2022 at 8:43 AM Roger Lu <roger.lu@...iatek.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
>
>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics

Powered by blists - more mailing lists