Re: [PATCH v1 6/9] phy: qcom-qmp: Add support for DP in USB3+DP combo phy

[Stephen Boyd]

Quoting Dmitry Baryshkov (2020-09-01 06:36:34)
> On 26/08/2020 05:47, Stephen Boyd wrote:
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > index 76d7a9e80f04..dd77c7dfa310 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> > @@ -947,6 +947,130 @@ static const struct qmp_phy_init_tbl qmp_v3_usb3_tx_tbl[] = {
> >       QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_TX, 0x06),
> >   };
> >   
> 
> I'd suggest to split common part of the following tables into 
> dpphy_cfg->serdes_tbl and add the rest as "addon tables" 
> (serdes_tbl_rbr, serdes_rbl_hbr/2/3) into the same dpphy_cfg.
> It would ease V4 QMP DP PHY support.

Ok. I tried to avoid doing that initially in case something is wrong
from the copy over from the DP driver. Also it means the sequence of
writes is different order but I don't think that matters.

> 
> > +static const struct qmp_phy_init_tbl qmp_v3_dp_serdes_tbl_rbr[] = {
> > +     QMP_PHY_INIT_CFG(QSERDES_V3_COM_SVS_MODE_CLK_SEL, 0x01),
> > +     QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYSCLK_EN_SEL, 0x37),
> > +     QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYS_CLK_CTRL, 0x02),
> > +     QMP_PHY_INIT_CFG(QSERDES_V3_COM_CLK_ENABLE1, 0x0e),
> > +     QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYSCLK_BUF_ENABLE, 0x06),
> > +     QMP_PHY_INIT_CFG(QSERDES_V3_COM_CLK_SELECT, 0x30),
> > +     QMP_PHY_INIT_CFG(QSERDES_V3_COM_CMN_CONFIG, 0x02),
> > +     QMP_PHY_INIT_CFG(QSERDES_V3_COM_HSCLK_SEL, 0x0c),
[...]
> > @@ -2475,6 +2613,329 @@ static void qcom_qmp_phy_configure(void __iomem *base,
> >       qcom_qmp_phy_configure_lane(base, regs, tbl, num, 0xff);
> >   }
> >   
> > +static int qcom_qmp_phy_serdes_init(struct qmp_phy *qphy)
> > +{
> > +     struct qcom_qmp *qmp = qphy->qmp;
> > +     const struct qmp_phy_cfg *cfg = qphy->cfg;
> > +     void __iomem *serdes = qphy->serdes;
> > +     const struct phy_configure_opts_dp *dp_opts = &qphy->dp_opts;
> > +     const struct qmp_phy_init_tbl *serdes_tbl;
> > +     int serdes_tbl_num;
> > +     int ret;
> > +
> > +     if (cfg->type == PHY_TYPE_DP) {
> > +             switch (dp_opts->link_rate) {
> > +             case 1620:
> > +                     serdes_tbl = qmp_v3_dp_serdes_tbl_rbr;
> > +                     serdes_tbl_num = ARRAY_SIZE(qmp_v3_dp_serdes_tbl_rbr);
> > +                     break;
> > +             case 2700:
> > +                     serdes_tbl = qmp_v3_dp_serdes_tbl_hbr;
> > +                     serdes_tbl_num = ARRAY_SIZE(qmp_v3_dp_serdes_tbl_hbr);
> > +                     break;
> > +             case 5400:
> > +                     serdes_tbl = qmp_v3_dp_serdes_tbl_hbr2;
> > +                     serdes_tbl_num = ARRAY_SIZE(qmp_v3_dp_serdes_tbl_hbr2);
> > +                     break;
> > +             case 8100:
> > +                     serdes_tbl = qmp_v3_dp_serdes_tbl_hbr3;
> > +                     serdes_tbl_num = ARRAY_SIZE(qmp_v3_dp_serdes_tbl_hbr3);
> > +                     break;
> > +             default:
> > +                     /* Other link rates aren't supported */
> > +                     return -EINVAL;
> > +             }
> > +     } else {
> > +             serdes_tbl = cfg->serdes_tbl;
> > +             serdes_tbl_num = cfg->serdes_tbl_num;
> > +     }
> > +     qcom_qmp_phy_configure(serdes, cfg->regs, serdes_tbl, serdes_tbl_num);
> 
> If we split DP serdes tables, it would look lile:
>         qcom_qmp_phy_configure(serdes, cfg->regs, cfg->serdes_tbl, 
> cfg->serdes_tbl_num);
>         if (cfg->type == PHY_TYPE_DP) {
>                 switch (dp_opts->link_rate) {
>                 case 1620:
>                         qcom_qmp_phy_configure(serdes, cfg->regs, cfg->serdes_tbl_rbr, 
> cfg->serdes_tbl_rbr_num);
>                         break;
>                 case 2700:
>                         qcom_qmp_phy_configure(serdes, cfg->regs, cfg->serdes_tbl_hbr, 
> cfg->serdes_tbl_hbr_num);
>                         break;
>                 case 5400:
>                         qcom_qmp_phy_configure(serdes, cfg->regs, cfg->serdes_tbl_hbr2, 
> cfg->serdes_tbl_hbr2_num);
>                         break;
>                 case 8100:
>                         qcom_qmp_phy_configure(serdes, cfg->regs, cfg->serdes_tbl_hbr3, 
> cfg->serdes_tbl_hbr3_num);
>                         break;
>                 default:
>                         /* Other link rates aren't supported */
>                         return -EINVAL;
>                 }
>         }

Ok, sure!

> 
> 
>  > +    qcom_qmp_phy_configure(serdes, cfg->regs, serdes_tbl, serdes_tbl_num);
> 
> 
> > +
> > +     if (cfg->has_phy_com_ctrl) {
> > +             void __iomem *status;
> > +             unsigned int mask, val;
> > +
> > +             qphy_clrbits(serdes, cfg->regs[QPHY_COM_SW_RESET], SW_RESET);
> > +             qphy_setbits(serdes, cfg->regs[QPHY_COM_START_CONTROL],
> > +                          SERDES_START | PCS_START);
> > +
> > +             status = serdes + cfg->regs[QPHY_COM_PCS_READY_STATUS];
> > +             mask = cfg->mask_com_pcs_ready;
> > +
> > +             ret = readl_poll_timeout(status, val, (val & mask), 10,
> > +                                      PHY_INIT_COMPLETE_TIMEOUT);
> > +             if (ret) {
> > +                     dev_err(qmp->dev,
> > +                             "phy common block init timed-out\n");
> > +                     return ret;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static void qcom_qmp_phy_dp_aux_init(struct qmp_phy *qphy)
> > +{
> > +     writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN |
> > +            DP_PHY_PD_CTL_PLL_PWRDN | DP_PHY_PD_CTL_DP_CLAMP_EN,
> > +            qphy->pcs + QSERDES_V3_DP_PHY_PD_CTL);
> > +
> > +     /* Turn on BIAS current for PHY/PLL */
> > +     writel(QSERDES_V3_COM_BIAS_EN | QSERDES_V3_COM_BIAS_EN_MUX |
> > +            QSERDES_V3_COM_CLKBUF_L_EN | QSERDES_V3_COM_EN_SYSCLK_TX_SEL,
> > +            qphy->serdes + QSERDES_V3_COM_BIAS_EN_CLKBUFLR_EN);
> > +
> > +     writel(DP_PHY_PD_CTL_PSR_PWRDN, qphy->pcs + QSERDES_V3_DP_PHY_PD_CTL);
> > +
> > +     /* Make sure that hardware is done with  PSR power down */
> > +     wmb();
> > +     writel(DP_PHY_PD_CTL_PWRDN | DP_PHY_PD_CTL_AUX_PWRDN |
> > +            DP_PHY_PD_CTL_LANE_0_1_PWRDN |
> > +            DP_PHY_PD_CTL_LANE_2_3_PWRDN | DP_PHY_PD_CTL_PLL_PWRDN |
> > +            DP_PHY_PD_CTL_DP_CLAMP_EN,
> > +            qphy->pcs + QSERDES_V3_DP_PHY_PD_CTL);
> > +
> > +     writel(QSERDES_V3_COM_BIAS_EN |
> > +            QSERDES_V3_COM_BIAS_EN_MUX | QSERDES_V3_COM_CLKBUF_R_EN |
> > +            QSERDES_V3_COM_CLKBUF_L_EN | QSERDES_V3_COM_EN_SYSCLK_TX_SEL |
> > +            QSERDES_V3_COM_CLKBUF_RX_DRIVE_L,
> > +            qphy->serdes + QSERDES_V3_COM_BIAS_EN_CLKBUFLR_EN);
> > +
> > +     writel(0x00, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG0);
> > +     writel(0x13, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG1);
> > +     writel(0x24, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG2);
> > +     writel(0x00, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG3);
> > +     writel(0x0a, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG4);
> > +     writel(0x26, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG5);
> > +     writel(0x0a, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG6);
> > +     writel(0x03, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG7);
> > +     writel(0xbb, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG8);
> > +     writel(0x03, qphy->pcs + QSERDES_V3_DP_PHY_AUX_CFG9);
> > +     qphy->dp_aux_cfg = 0;
> > +
> > +     writel(PHY_AUX_STOP_ERR_MASK | PHY_AUX_DEC_ERR_MASK |
> > +            PHY_AUX_SYNC_ERR_MASK | PHY_AUX_ALIGN_ERR_MASK |
> > +            PHY_AUX_REQ_ERR_MASK,
> > +            qphy->pcs + QSERDES_V3_DP_PHY_AUX_INTERRUPT_MASK);
> > +}
> > +
> > +static const u8 vm_pre_emphasis[4][4] = {
> 
> Could you please prefix with v3? V4 will use different tables here

Done.

> 
> > +     { 0x00, 0x0b, 0x12, 0xff },       /* pe0, 0 db */
> > +     { 0x00, 0x0a, 0x12, 0xff },       /* pe1, 3.5 db */
> > +     { 0x00, 0x0c, 0xff, 0xff },       /* pe2, 6.0 db */
> > +     { 0xff, 0xff, 0xff, 0xff }        /* pe3, 9.5 db */
> > +};
> > +
> > +/* voltage swing, 0.2v and 1.0v are not support */
> > +static const u8 vm_voltage_swing[4][4] = {
> > +     { 0x07, 0x0f, 0x14, 0xff }, /* sw0, 0.4v  */
> > +     { 0x11, 0x1d, 0x1f, 0xff }, /* sw1, 0.6 v */
> > +     { 0x18, 0x1f, 0xff, 0xff }, /* sw1, 0.8 v */
> > +     { 0xff, 0xff, 0xff, 0xff }  /* sw1, 1.2 v, optional */
> > +};
> > +
> > +static const u8 vm_pre_emphasis_hbr3_hbr2[4][4] = {
> > +     { 0x00, 0x0c, 0x15, 0x1a },
> > +     { 0x02, 0x0e, 0x16, 0xff },
> > +     { 0x02, 0x11, 0xff, 0xff },
> > +     { 0x04, 0xff, 0xff, 0xff }
> > +};
> > +
> > +static const u8 vm_voltage_swing_hbr3_hbr2[4][4] = {
> > +     { 0x02, 0x12, 0x16, 0x1a },
> > +     { 0x09, 0x19, 0x1f, 0xff },
> > +     { 0x10, 0x1f, 0xff, 0xff },
> > +     { 0x1f, 0xff, 0xff, 0xff }
> > +};
> > +
> > +static const u8 vm_pre_emphasis_hbr_rbr[4][4] = {
> > +     { 0x00, 0x0c, 0x14, 0x19 },
> > +     { 0x00, 0x0b, 0x12, 0xff },
> > +     { 0x00, 0x0b, 0xff, 0xff },
> > +     { 0x04, 0xff, 0xff, 0xff }
> > +};
> > +
> > +static const u8 vm_voltage_swing_hbr_rbr[4][4] = {
> > +     { 0x08, 0x0f, 0x16, 0x1f },
> > +     { 0x11, 0x1e, 0x1f, 0xff },
> > +     { 0x19, 0x1f, 0xff, 0xff },
> > +     { 0x1f, 0xff, 0xff, 0xff }
> > +};
> > +
> > +static void qcom_qmp_phy_configure_dp_tx(struct qmp_phy *qphy)
> 
> With these functions I'm struggling between introducing 
> PHY_TYPE_DP_V3/V4 and introducing callbacks into qmp_phy_cfg. What would 
> you prefer?
> 
> What about the following struct?
> 
> struct qmp_phy_dp_opts {
>         void (*dp_aux_init)(struct qmp_phy *qphy);
>         void (*dp_configure_tx)(struct qmp_phy *qphy);
>         void (*dp_configure_lanes)(struct qmp_phy *qphy);
> };
> 
> I'm not sure about dp_calibrate().
> 

Is there v4 code somewhere that I can see? Another level of indirection
is always a solution, so it is probably fine. This driver is currently
written with many conditionals instead of function tables so I'm not
sure it fits in with the style of how things are done though. The
alternative is to use an enum and call different functions?

The calibrate call is there to "turn the crank" on the aux settings.  I
need to cycle through the different values for that aux register so that
aux can be tuned properly. The AUX channel really has another phy that
needs tuning so we're sort of combining the aux and DP link phy together
here by letting the calibrate call tune the AUX phy and the configure
call tune the DP phy. I don't see any sort of concept of an AUX phy
though so this seemed ok. Does v4 need to tune more registers?