Message-ID: <mrsdwtzz7x6uuoakjr6kymqccfxs3lndplols7j33apbru7jii@tjdljisgaqny>
Date: Mon, 18 Aug 2025 12:39:43 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
Cc: Bjorn Helgaas <helgaas@...nel.org>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>, 
	Jingoo Han <jingoohan1@...il.com>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Rob Herring <robh@...nel.org>, Jeff Johnson <jjohnson@...nel.org>, 
	Bartosz Golaszewski <brgl@...ev.pl>, Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	mhi@...ts.linux.dev, linux-wireless@...r.kernel.org, ath11k@...ts.infradead.org, 
	qiang.yu@....qualcomm.com, quic_vbadigan@...cinc.com, quic_vpernami@...cinc.com, 
	quic_mrana@...cinc.com, Jeff Johnson <jeff.johnson@....qualcomm.com>
Subject: Re: [PATCH v4 02/11] PCI/bwctrl: Add support to scale bandwidth
 before & after link re-training

On Wed, Aug 13, 2025 at 09:25:03AM GMT, Krishna Chaitanya Chundru wrote:
> 
> 
> On 8/12/2025 10:13 PM, Manivannan Sadhasivam wrote:
> > On Tue, Aug 12, 2025 at 09:35:46AM GMT, Krishna Chaitanya Chundru wrote:
> > > 
> > > 
> > > On 7/22/2025 4:33 PM, Krishna Chaitanya Chundru wrote:
> > > > 
> > > > 
> > > > On 7/12/2025 4:36 AM, Krishna Chaitanya Chundru wrote:
> > > > > 
> > > > > 
> > > > > On 7/12/2025 3:06 AM, Bjorn Helgaas wrote:
> > > > > > On Mon, Jun 09, 2025 at 04:21:23PM +0530, Krishna Chaitanya
> > > > > > Chundru wrote:
> > > > > > > If the driver wants to move to higher data rate/speed than
> > > > > > > the current data
> > > > > > > rate then the controller driver may need to change certain
> > > > > > > votes so that
> > > > > > > link may come up at requested data rate/speed like QCOM PCIe
> > > > > > > controllers
> > > > > > > need to change their RPMh (Resource Power Manager-hardened) state. Once
> > > > > > > link retraining is done controller drivers needs to adjust their votes
> > > > > > > based on the final data rate.
> > > > > > > 
> > > > > > > Some controllers also may need to update their bandwidth voting like
> > > > > > > ICC BW votings etc.
> > > > > > > 
> > > > > > > So, add pre_link_speed_change() & post_link_speed_change() op to call
> > > > > > > before & after the link re-train. There is no explicit
> > > > > > > locking mechanisms
> > > > > > > as these are called by a single client Endpoint driver.
> > > > > > > 
> > > > > > > In case of PCIe switch, if there is a request to change
> > > > > > > target speed for a
> > > > > > > downstream port then no need to call these function ops as these are
> > > > > > > outside the scope of the controller drivers.
> > > > > > 
> > > > > > > +++ b/include/linux/pci.h
> > > > > > > @@ -599,6 +599,24 @@ struct pci_host_bridge {
> > > > > > >        void (*release_fn)(struct pci_host_bridge *);
> > > > > > >        int (*enable_device)(struct pci_host_bridge *bridge,
> > > > > > > struct pci_dev *dev);
> > > > > > >        void (*disable_device)(struct pci_host_bridge *bridge,
> > > > > > > struct pci_dev *dev);
> > > > > > > +    /*
> > > > > > > +     * Callback to the host bridge drivers to update ICC BW
> > > > > > > votes, clock
> > > > > > > +     * frequencies etc.. for the link re-train to come up
> > > > > > > in targeted speed.
> > > > > > > +     * These are intended to be called by devices directly
> > > > > > > attached to the
> > > > > > > +     * Root Port. These are called by a single client
> > > > > > > Endpoint driver, so
> > > > > > > +     * there is no need for explicit locking mechanisms.
> > > > > > > +     */
> > > > > > > +    int (*pre_link_speed_change)(struct pci_host_bridge *bridge,
> > > > > > > +                     struct pci_dev *dev, int speed);
> > > > > > > +    /*
> > > > > > > +     * Callback to the host bridge drivers to adjust ICC BW
> > > > > > > votes, clock
> > > > > > > +     * frequencies etc.. to the updated speed after link
> > > > > > > re-train. These
> > > > > > > +     * are intended to be called by devices directly attached to the
> > > > > > > +     * Root Port. These are called by a single client Endpoint driver,
> > > > > > > +     * so there is no need for explicit locking mechanisms.
> > > > > > 
> > > > > > No need to repeat the entire comment.  s/.././
> > > > > > 
> > > > > > These pointers feel awfully specific for being in struct
> > > > > > pci_host_bridge, since we only need them for a questionable QCOM
> > > > > > controller.  I think this needs to be pushed down into qcom somehow as
> > > > > > some kind of quirk.
> > > > > > 
> > > > > Currently these are needed by QCOM controllers, but it may also needed
> > > > > by other controllers may also need these for updating ICC votes, any
> > > > > system level votes, clock frequencies etc.
> > > > > QCOM controllers is also doing one extra step in these functions to
> > > > > disable and enable ASPM only as it cannot link speed change support
> > > > > with ASPM enabled.
> > > > > 
> > > > Bjorn, can you check this.
> > > > 
> > > > For QCOM devices we need to update the RPMh vote i.e a power source
> > > > votes for the link to come up in required speed. and also we need
> > > > to update interconnect votes also. This will be applicable for
> > > > other vendors also.
> > > > 
> > > > If this is not correct place I can add them in the pci_ops.
> > > Bjorn,
> > > 
> > > Can you please comment on this.
> > > 
> > > Is this fine to move these to the pci_ops of the bridge.
> > > Again these are not specific to QCOM, any controller driver which
> > > needs to change their clock rates, ICC bw votes etc needs to have
> > > these.
> > > 
> > 
> > No, moving to 'pci_ops' is terrible than having it in 'pci_host_bridge' IMO. If
> > we want to get rid of these ops, we can introduce a quirk flag in
> > 'pci_host_bridge' and when set, the bwctrl code can disable/enable ASPM
> > before/after link retrain. This clearly states that the controller is quirky and
> > we need to disable/enable ASPM.
> > 
> > For setting OPP, you can have a separate callback in 'pci_host_bridge' that just
> > allows setting OPP *after* retrain, like 'pci_host_bridge:link_change_notify()'.
> > I don't think you really need to set OPP before retrain though. As even if you
> > do it pre and post link retrain, there is still a small window where the link
> > will operate without adequate vote.
> > 
> Hi Mani,
> 
> we need to update the OPP votes before link retrain, for example if
> there is request  to change data rate from 5 GT/s to 8 GT/s on some
> platforms we need to update RPMh votes from low_svs to NOM corner
> without this clocks will not scale for data rates 8 GT/s and link
> retrain will fail. For that reason we are trying to add pre and post
> callbacks.
> 

If we are targetting OPP only, then can't we do direct OPP setting from the
bwctrl driver itself? Or we also need to set ICC votes directly also (in the
non-opp case)?

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists