Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
In-Reply-To: <Y8ERRMXng+kL+Vtc@hovoldconsulting.com>
References: <20230113005405.3992011-1-swboyd@chromium.org> <20230113005405.3992011-4-swboyd@chromium.org>
 <Y8ERRMXng+kL+Vtc@hovoldconsulting.com>
From:   Stephen Boyd <swboyd@chromium.org>
User-Agent: alot/0.10
Date:   Fri, 13 Jan 2023 14:03:00 -0600
Message-ID: <CAE-0n52+_wuX_8nuDDex-xnmg22ZLBUedqSxG1VFu+yVPbv9PQ@mail.gmail.com>
Subject: Re: [PATCH 5.15.y 3/4] phy: qcom-qmp-combo: fix broken power on
To:     Johan Hovold <johan@kernel.org>
Cc:     stable@vger.kernel.org, Johan Hovold <johan+linaro@kernel.org>,
        linux-kernel@vger.kernel.org, patches@lists.linux.dev,
        Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
        Vinod Koul <vkoul@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Quoting Johan Hovold (2023-01-13 00:07:32)
> On Thu, Jan 12, 2023 at 04:54:04PM -0800, Stephen Boyd wrote:
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index c6f860ce3d99..9fda6d283f20 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -4620,13 +4621,13 @@ static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
> >               qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
> >                            SW_PWRDN);
> >       } else {
> > -             if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> > -                     qphy_setbits(pcs,
> > -                                     cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> > -                                     cfg->pwrdn_ctrl);
> > +             if (usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> > +                     qphy_setbits(usb_phy->pcs,
> > +                                     usb_phy->cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> > +                                     usb_phy->cfg->pwrdn_ctrl);
> >               else
> > -                     qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL,
> > -                                     cfg->pwrdn_ctrl);
> > +                     qphy_setbits(usb_phy->pcs, QPHY_POWER_DOWN_CONTROL,
> > +                                     usb_phy->cfg->pwrdn_ctrl);
>
> This bit looks like it requires some more work in order not to break
> PCIe and UFS PHYs, which lack the USB registers.

Good point. I don't have PCIe or UFS PHYs so I missed this.

>
> >       }
> >
> >       mutex_unlock(&qmp->phy_mutex);
> > @@ -5769,6 +5770,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
> >                       goto err_node_put;
> >               }
> >
> > +             if (cfg->type == PHY_TYPE_USB3)

I changed this to by 'cfg->type != PHY_TYPE_DP'

> > +                     qmp->usb_phy = qmp->phys[id];
> > +
> >               /*
> >                * Register the pipe clock provided by phy.
> >                * See function description to see details of this pipe clock.
> > @@ -5791,6 +5795,9 @@ static int qcom_qmp_phy_probe(struct platform_device *pdev)
> >               id++;
> >       }
> >
> > +     if (!qmp->usb_phy)
> > +             return -EINVAL;
> > +
>
> This will break probe of PCIe and UFS PHYs unless you only check this
> for combo PHYs.
>
> Perhaps you can rename the usb_phy pointer to something more generic and
> set it also for PCIe and UFS so that it always points to the register
> block that should be used for QPHY_PCS_POWER_DOWN_CONTROL.

I worry that future backports will be complicated if they use 'usb_phy'.
Maybe I'll just leave this as is.