Message-ID: <CA+-6iNwAfur96=kftP_pqZDGUoGkb3_rjKnxiGJmL4xxmzTNaA@mail.gmail.com>
Date: Tue, 27 Aug 2024 11:01:37 -0400
From: Jim Quinlan <james.quinlan@...adcom.com>
To: Stanimir Varbanov <svarbanov@...e.de>
Cc: linux-pci@...r.kernel.org, Nicolas Saenz Julienne <nsaenz@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Lorenzo Pieralisi <lorenzo.pieralisi@....com>, 
	Cyril Brulebois <kibi@...ian.org>, Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>, 
	Krzysztof Kozlowski <krzk@...nel.org>, bcm-kernel-feedback-list@...adcom.com, 
	jim2101024@...il.com, Florian Fainelli <florian.fainelli@...adcom.com>, 
	Lorenzo Pieralisi <lpieralisi@...nel.org>, Krzysztof Wilczyński <kw@...ux.com>, 
	Rob Herring <robh@...nel.org>, Philipp Zabel <p.zabel@...gutronix.de>, 
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-rpi-kernel@...ts.infradead.org>, 
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" <linux-arm-kernel@...ts.infradead.org>, 
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 05/13] PCI: brcmstb: Use bridge reset if available

On Tue, Aug 27, 2024 at 8:28 AM Stanimir Varbanov <svarbanov@...e.de> wrote:
>
> Hi Jim,
>
> On 8/26/24 17:17, Jim Quinlan wrote:
> > On Mon, Aug 26, 2024 at 6:43 AM Stanimir Varbanov <svarbanov@...e.de> wrote:
> >>
> >> Hi Jim,
> >>
> >> <cut>
> >>
> >>>
> >>> Hi Stan,
> >>>
> >>> Most of the clocks on the STB chips come up active so one does not
> >>> have to turn them on and off to have the device function.  It helps
> >>> power savings to do this although I'm not sure it is significant.
> >>>>
> >>>>>
> >>>>> Perhaps you don't see the dependence on the PCIe clocks if the 2712
> >>>>> does not give the PCIe node a clock property and instead keeps its
> >>>>> clocks on all of the time.  In that case I would think that your
> >>>>> solution would be fine.
> >>>>
> >>>> What you mean by my solution? The one where avoiding assert of
> >>>> bridge_reset at link [1] bellow?
> >>>
> >>> Yes.
> >>>>
> >>>> If so, I still cannot understand the relation between bridge_reset and
> >>>> rescal as the comment mentions:
> >>>>
> >>>> "Shutting down this bridge on pcie1 means accesses to rescal block will
> >>>> hang the chip if another RC wants to assert/deassert rescal".
> >>>
> >>> I was just describing my observations; this should not be happening.
> >>> I would say it is a HW bug for the 2712.  I can file a bug against the
> >>> 2712 but that will not help us right now.  From what I was told by HW,
> >>> asserting the PCIe1 bridge reset does not affect the rescal settings,
> >>> but it does freeze access to the rescal registers, and that is game
> >>> over for the other PCIe controllers accessing the rescal registers.
> >>
> >> Good findings, thank you.
> >>
> >> The problem comes from this snippet from brcm_pcie_probe() :
> >>
> >>         ret = pci_host_probe(bridge);
> >>         if (!ret && !brcm_pcie_link_up(pcie))
> >>                 ret = -ENODEV;
> >>
> >>         if (ret) {
> >>                 brcm_pcie_remove(pdev);
> >>                 return ret;
> >>         }
> >>
> >> Even when pci_host_probe() is successful the .probe will fail if there
> >> are no endpoint devices on this root port bus. This is the case when
> >> probing pcie1 port which is the one with external connector. Cause the
> >> probe is failing we call reset_control_rearm(rescal) from
> >> brcm_pcie_remove(), after that during .probe of pcie2 (the root port
> >> where RP1 south-bridge is attached) reset_control_reset(rescal) will
> >> issue rescal reset thus rescal-reset driver will stuck on read/write
> >> registers.
> >>
> >> I think we have to drop this link-up check and allow the probe to finish
> >> successfully. Even that there no PCI devices attached to bus we want the
> >> root port to be visible by lspci tool.
> >
> > Hi Stan,
> >
> > What is gained by having only the root bridge shown by lspci?  We do
> > not support PCIe hotplug so why have lspci reporting a bridge with no
> > devices?
> >
> > The reason we do this is to save power -- when we see no device we
> > turn off the clocks, put things in reset (e.g. bridge), and turn off
> > the regulators.  We have SoCs with multiple controllers  and they
> > cannot afford to be supplying power to controllers with non-populated
> > sockets; these may be products that are trying to conform to mandated
> > energy-mode specifications.
>
> I totally agree, although I do not see power consumption significantly
> increased. Also I checked all other PCI controller drivers and no one
> else doing this.

Hi Stan,
I  see a few drivers use runtime_pm but we  do not; our architecture
is old and not amenable to that system.  Discounting runtime_pm, we
seem to be the only controller that uses the .suspend* or .resume*
methods.

I think we are kind of unique in how we turn off/on voltage regulators
for the endpoint device -- we put the regulator node under the port
driver DT node.  This is what customers asked for and what made
customers happy -- they just add a regulator node and ref to the DT we
provide and they don't have to worry about syncing power for their
device on their board.

There was also a recent patch submission that implemented something
similar by making the port driver customizable so that it could pick
up regulators; I don't recall if that ever got accepted.  But if it
was, we were thinking of switching to it.

Let me ask the HW folks if it is acceptable to leave the bridge reset
unasserted on remove() or suspend().  Note that if RPi decides to give
<clocks> and <clock-names>  valid props then problem will still be
there.

Regards,
Jim Quiinlan
Broad


>
> >
> >  This will solve partially the
> >> issue with accessing rescal reset-controller registers after asserting
> >> bridge_reset. The other part of the problem will be solved by remove the
> >> invocation of reset_control_rearm(rescal) from __brcm_pcie_remove().
> >> That way only the first probed root port will issue rescal reset and
> >> every next probe will not try to reset rescal because we do not call
> >> _rearm(rescal).
> >
> > In theory I agree with the above -- it should probably not be invoking
> > the rearm() when it is being deactivated.  However, I am concerned
> > about a controller(s) going into s2 suspend/resume or unbind/bind.
>
> In fact not calling rearm() from __brcm_pcie_remove() is not enough
> because the .probe will fail thus rescal reset will loose it's instance
> and next probed pcie root port will issue rescal reset again.
>
> I'd stick with avoiding assert of bridge_reset from brcm_pcie_turn_off()
> for now, and this will be true only for bcm2712.
>
> ~Stan

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4210 bytes)

Powered by blists - more mailing lists