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