Date:   Mon, 23 Jan 2023 15:00:14 +0100
From:   Stephan Gerhold <stephan@gerhold.net>
To:     Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc:     agross@kernel.org, andersson@kernel.org, konrad.dybcio@linaro.org,
        djakov@kernel.org, robh+dt@kernel.org,
        krzysztof.kozlowski+dt@linaro.org, linux-arm-msm@vger.kernel.org,
        linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, benl@squareup.com,
        shawn.guo@linaro.org, fabien.parent@linaro.org, leo.yan@linaro.org,
        dmitry.baryshkov@linaro.org
Subject: Re: [PATCH v4 0/6] Add MSM8939 SoC support with two devices
Message-ID: <Y86S7h/QxqXoKS1U@gerhold.net>
References: <20230123023127.1186619-1-bryan.odonoghue@linaro.org>
 <42baa874-c926-9111-b0b3-2df2562d8de6@linaro.org>
 <Y86CPmgvAi+kChQI@gerhold.net>
 <45800033-e2ae-09c8-b8a2-e97afb6508fd@linaro.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <45800033-e2ae-09c8-b8a2-e97afb6508fd@linaro.org>
Precedence: bulk

On Mon, Jan 23, 2023 at 01:23:22PM +0000, Bryan O'Donoghue wrote:
> On 23/01/2023 12:49, Stephan Gerhold wrote:
> > It's only required though because you forgot to drop the DT schema patch
> > (3/4) when I suggested half a year ago that you make the MSM8939
> > cpufreq-qcom-nvmem changes together with the CPR stack [1]. :/
> 
> Didn't forget, tested that and as I recall there are side-effects removing
> 8939 from drivers/cpufreq/cpufreq-dt-platdev.c - not all processors were
> booted.
> 

The cpufreq-dt-platdev.c addition for MSM8939 does not exist upstream
because you dropped it in your v3 back then. You just kept the DT schema
part. I don't have that addition and have no problems with SMP boot so
I would say it works fine without.

> > Anyway, it looks like qcom-cpufreq-nvmem.yaml requiring "cpr" power
> > domain unconditionally is a mistake anyway for multiple platforms.
> > [2] was recently submitted to fix this so that patch should allow you to
> > drop the dummy nodes. ????
> > 
> > [1]:https://lore.kernel.org/linux-arm-msm/Ysf8VRaXdGg+8Ev3@gerhold.net/
> > [2]:https://lore.kernel.org/linux-arm-msm/20230122174548.13758-1-ansuelsmth@gmail.com/
> 
> 8939 _is_ a CPR device, I think qcs404 comes from msm893x IP.
> 
> To me it makes more sense to stub CPR in the DTS than to, wrongly declare
> the 8939 a non-CPR device.
> 

It is not clear yet which power domains 8939 needs to list for the CPUs.
The conclusion of the previous discussion of CPR for MSM8916 was that
the VDDMX requirements would be best handled separately from the CPR
driver, by listing it as separate power domain for all CPUs [3].

Unless this conclusion changes with your CPR patch set this means that
both the DTS and the DT schema will need changes anyway, because you
wouldn't need power-domain-names = "cpr", but rather

	power-domains = <&rpmpd MSM8939_VDDMX_AO>, <&vreg_dummy>;
	power-domain-names = "mx", "cpr";

QCS404 is a different situation in this case because it does not have
the requirement of voting for VDDMX states.

IMO this means that listing only "cpr" there with a dummy node is more
confusing than helpful right now. (I can explain this further if you
want, but I think I tend to write too long answers...)

Thanks,
Stephan

[3]: https://lore.kernel.org/linux-arm-msm/20200403175934.GA96064@gerhold.net/