Re: [PATCH 1/2] interconnect: qcom: sc7180: Drop IP0 interconnects

From: Stephen Boyd <swboyd@chromium.org>
To: Alex Elder <elder@linaro.org>, Doug Anderson <dianders@chromium.org>
Cc: Georgi Djakov <djakov@kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	 patches@lists.linux.dev,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	 Linux PM <linux-pm@vger.kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	 Taniya Das <quic_tdas@quicinc.com>,
	Mike Tipton <quic_mdtipton@quicinc.com>
Subject: Re: [PATCH 1/2] interconnect: qcom: sc7180: Drop IP0 interconnects
Date: Wed, 13 Apr 2022 16:14:02 -0700	[thread overview]
Message-ID: <CAE-0n53kebRHTogSkiAOcA4tMpA+EcXQHtNX0Zzut-xE2vCkpw@mail.gmail.com> (raw)
In-Reply-To: <d80ece40-fdd7-f304-3989-47ae152d5ac9@linaro.org>

Quoting Alex Elder (2022-04-13 14:02:00)
> On 4/13/22 3:55 PM, Doug Anderson wrote:
> > Hi,
> >
> > On Tue, Apr 12, 2022 at 4:20 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >>
> >> @@ -519,8 +500,6 @@ static const struct of_device_id qnoc_of_match[] = {
> >>            .data = &sc7180_dc_noc},
> >>          { .compatible = "qcom,sc7180-gem-noc",
> >>            .data = &sc7180_gem_noc},
> >> -       { .compatible = "qcom,sc7180-ipa-virt",
> >> -         .data = &sc7180_ipa_virt},
> >>          { .compatible = "qcom,sc7180-mc-virt",
> >>            .data = &sc7180_mc_virt},
> >>          { .compatible = "qcom,sc7180-mmss-noc",
> >
> > I have no objection to ${SUBJECT} change landing and based on all your
> > research and Alex's review/testing I think it's good to go.
> >
> > However, now that you're removed the driver that cared about
> > "qcom,sc7180-ipa-virt", should we also be removing it from the
> > `bindings/interconnect/qcom,rpmh.yaml` file and the `sc7180.dtsi`
> > file? I think that removing it from _either_ the driver (like your
> > patch here does) _or_ the sc7180.dtsi file would fix the bug, right?
> > ...and then removing it from the yaml would just be cleanup...

Yes, but that's mostly a cleanup. I didn't include it in this series
because DTB is supposed to be "stable" and thus backporting a fix to the
kernel by removing something from DT is sort of wrong. I don't know or
expect that the kernel DTS files will be used from the stable kernels.
It's better to fix the kernel C code. We can of course remove the
binding, but there's a part of me that would prefer that we put the IPA
clk back into the interconnect driver, so leaving the binding is another
motivator for me to hopefully excise the IPA clk from the rpmh-clk
driver in the future.

Anyway, I'm happy to remove the compatible string from the binding if
folks want that. Having the DT node is wasteful because the kernel makes
a device so we can certainly remove that as well. I'll send another
patch for that if this patch is accepted by Georgi.

>
> That's a good point, I hadn't thought about that but you're right.
>
> I think we were too pleased about identifying the problem and
> proving it could happen (and cause a crash), so we didn't think
> hard enough about this other piece.
>
> Stephen, I think you should re-spin the series and add the
> proper change to the binding.  You can keep the tags I gave
> before.

I will not combine the removal of the binding from this patch. This
patch is good as is and fixes the problem while ignoring the DT binding
and that larger discussion.

>
> I've got a note to follow up with similar changes to other
> platforms where the interconnect driver includes resource "IP0"
> and will plan to do what Doug suggests there too.