Re: [PATCH 6/7] phy: qcom-qmp-pcie: add support for ipq9574 gen3x2 PHY

[Alex G.]

On 4/6/24 09:37, Dmitry Baryshkov wrote:
> On Sat, 6 Apr 2024 at 11:47, Vinod Koul <vkoul@xxxxxxxxxx> wrote:
> > On 02-04-24, 14:25, Alexandru Gagniuc wrote:
> > > Add support for the gen3x2 PCIe PHY on IPQ9574, ported form downstream
> > > 5.4 kernel. Only the serdes and pcs_misc tables are new, the others
> > > being reused from IPQ8074 and IPQ6018 PHYs.
> > >
> > > Signed-off-by: Alexandru Gagniuc <mr.nuke.me@xxxxxxxxx>
> > > ---
> > >   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c      | 136 +++++++++++++++++-
> > >   .../phy/qualcomm/phy-qcom-qmp-pcs-pcie-v5.h   |  14 ++
> > >   2 files changed, 149 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > index 8836bb1ff0cc..f07bd27e3b7a 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > > @@ -487,6 +487,100 @@ static const struct qmp_phy_init_tbl ipq8074_pcie_gen3_pcs_misc_tbl[] = {
> > >        QMP_PHY_INIT_CFG(QPHY_V4_PCS_PCIE_ENDPOINT_REFCLK_DRIVE, 0xc1),
> > >   };
> > >
> > > +static const struct qmp_phy_init_tbl ipq9574_gen3x2_pcie_serdes_tbl[] = {
> > > +     QMP_PHY_INIT_CFG(QSERDES_PLL_BIAS_EN_CLKBUFLR_EN, 0x18),
> > > +     QMP_PHY_INIT_CFG(QSERDES_PLL_BIAS_EN_CTRL_BY_PSM, 0x01),
> > > +     QMP_PHY_INIT_CFG(QSERDES_PLL_CLK_SELECT, 0x31),
> > > +     QMP_PHY_INIT_CFG(QSERDES_PLL_PLL_IVCO, 0x0F),
> > > +     QMP_PHY_INIT_CFG(QSERDES_PLL_BG_TRIM, 0x0F),
> >
> > Lower case here and everywhere please
>
> For hex values

I will these updated in V2. Thanks!

> > >   static const struct qmp_phy_init_tbl sdm845_qmp_pcie_serdes_tbl[] = {
> > >        QMP_PHY_INIT_CFG(QSERDES_V3_COM_BIAS_EN_CLKBUFLR_EN, 0x14),
> > >        QMP_PHY_INIT_CFG(QSERDES_V3_COM_CLK_SELECT, 0x30),
> > > @@ -2448,7 +2542,7 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
> > >
> > >   /* list of clocks required by phy */
> > >   static const char * const qmp_pciephy_clk_l[] = {
> > > -     "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux",
> > > +     "aux", "cfg_ahb", "ref", "refgen", "rchng", "phy_aux", "anoc", "snoc"
> >
> > How about older platforms which dont have these clocks
>
> The driver uses devm_clk_bulk_get_optional(), so it should be fine.
> But the more important question should be why the platform needs
> anoc/snoc clocks here.

I got the info from the downstream 5.4 kernel. While I don't know why 
these new clocks are required, they are needed. Omitting them will cause 
the boot to hang.

I could rename them to "snoc_lane" and "anoc_lane", if you think that 
makes more sense.


Alex