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
--
மணிவண்ணன் சதாசிவம்
