On Wed, Jan 10, 2024 at 11:35:24AM +0530, Siddharth Vadapalli wrote:
> Hello Bjorn,
>
> On 10/01/24 02:53, Bjorn Helgaas wrote:
> > On Wed, Sep 27, 2023 at 09:48:45AM +0530, Siddharth Vadapalli wrote:
> >> The PCI driver invokes the PHY APIs using the ks_pcie_enable_phy()
> >> function. The PHY in this case is the Serdes. It is possible that the
> >> PCI instance is configured for 2 lane operation across two different
>
> ...
>
> >>
> >> + /* Obtain reference(s) to the phy(s) */
> >> + for (i = 0; i < num_lanes; i++)
> >> + phy_pm_runtime_get_sync(ks_pcie->phy[i]);
> >> +
> >> ret = ks_pcie_enable_phy(ks_pcie);
> >> +
> >> + /* Release reference(s) to the phy(s) */
> >> + for (i = 0; i < num_lanes; i++)
> >> + phy_pm_runtime_put_sync(ks_pcie->phy[i]);
> >
> > This looks good and has already been applied, so no immediate action
> > required.
> >
> > This is the only call to ks_pcie_enable_phy(), and these loops get and
> > put the PM references for the same PHYs initialized in
> > ks_pcie_enable_phy(), so it seems like maybe these loops could be
> > moved *into* ks_pcie_enable_phy().
>
> Does the following look fine?
> ===============================================================================
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c
> b/drivers/pci/controller/dwc/pci-keystone.c
> index e02236003b46..6e9f9589d26c 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -962,6 +962,9 @@ static int ks_pcie_enable_phy(struct keystone_pcie *ks_pcie)
> int num_lanes = ks_pcie->num_lanes;
>
> for (i = 0; i < num_lanes; i++) {
> + /* Obtain reference to the phy */
> + phy_pm_runtime_get_sync(ks_pcie->phy[i]);
I thought the point was that you needed to guarantee that all PHYs are
powered on and stay that way before initializing any of them. If so,
you would need a separate loop, e.g.,
for (i = 0; i < num_lanes; i++)
phy_pm_runtime_get_sync(ks_pcie->phy[i]);
for (i = 0; i < num_lanes; i++) {
ret = phy_reset(ks_pcie->phy[i]);
...
> ret = phy_reset(ks_pcie->phy[i]);
> if (ret < 0)
> goto err_phy;
> @@ -977,12 +980,18 @@ static int ks_pcie_enable_phy(struct keystone_pcie *ks_pcie)
> }
> }
>
> + /* Release reference(s) to the phy(s) */
> + for (i = 0; i < num_lanes; i++)
> + phy_pm_runtime_put_sync(ks_pcie->phy[i]);
> +
> return 0;
>
> err_phy:
> while (--i >= 0) {
> phy_power_off(ks_pcie->phy[i]);
> phy_exit(ks_pcie->phy[i]);
> + /* Release reference to the phy */
> + phy_pm_runtime_put_sync(ks_pcie->phy[i]);
> }
>
> return ret;
