Re: [PATCH v4 3/6] phy: qcom: qmp: Add phy register and clk setting for x1e80100 PCIe3

Johan Hovold
2024-09-24T15:15:29Z

On Tue, Sep 24, 2024 at 03:14:41AM -0700, Qiang Yu wrote:
> Currently driver supports only x4 lane based functionality using tx/rx and
> tx2/rx2 pair of register sets. To support 8 lane functionality with PCIe3,
> PCIe3 related QMP PHY provides additional programming which are available
> as txz and rxz based register set. Hence adds txz and rxz based registers
> usage and programming sequences.

> Phy register setting for txz and rxz will
> be applied to all 8 lanes. Some lanes may have different settings on
> several registers than txz/rxz, these registers should be programmed after
> txz/rxz programming sequences completing.

Please expand and clarify what you mean by this.

> Besides, x1e80100 SoC uses QMP phy with version v6.30 for PCIe Gen4 x8.
> Add the new register offsets in a dedicated header file.
>
> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Konrad Dybcio <konradybcio@kernel.org>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 211 ++++++++++++++++++
> .../qualcomm/phy-qcom-qmp-pcs-pcie-v6_30.h | 25 +++
> drivers/phy/qualcomm/phy-qcom-qmp-pcs-v6_30.h | 19 ++
> 3 files changed, 255 insertions(+)
> create mode 100644 drivers/phy/qualcomm/phy-qcom-qmp-pcs-pcie-v6_30.h
> create mode 100644 drivers/phy/qualcomm/phy-qcom-qmp-pcs-v6_30.h
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index f71787fb4d7e..d7bbd9df11d7 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c

> @@ -1344,6 +1346,155 @@ static const struct qmp_phy_init_tbl x1e80100_qmp_gen4x2_pcie_pcs_misc_tbl[] = {
> QMP_PHY_INIT_CFG(QPHY_PCIE_V6_20_PCS_G4_FOM_EQ_CONFIG5, 0x8a),
> };
>
> +static const struct qmp_phy_init_tbl x1e80100_qmp_gen4x8_pcie_txz_tbl[] = {

Please try to follow the sort order used for the other platforms for
these tables (e.g. serdes, tx, rx, pcr, pcr_misc).

> +static const struct qmp_phy_init_tbl x1e80100_qmp_gen4x8_pcie_pcs_misc_tbl[] = {
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_ENDPOINT_REFCLK_DRIVE, 0xc1),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_OSC_DTCT_ACTIONS, 0x00),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_EQ_CONFIG1, 0x16),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_G4_EQ_CONFIG5, 0x02),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_G4_PRE_GAIN, 0x2e),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_RX_MARGINING_CONFIG1, 0x03),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_RX_MARGINING_CONFIG3, 0x28),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_RX_MARGINING_CONFIG5, 0x18),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_G3_FOM_EQ_CONFIG5, 0x7a),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_G4_FOM_EQ_CONFIG5, 0x8a),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_G3_RXEQEVAL_TIME, 0x27),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_G4_RXEQEVAL_TIME, 0x27),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_TX_RX_CONFIG, 0xc0),
> + QMP_PHY_INIT_CFG(QPHY_PCIE_V6_30_PCS_POWER_STATE_CONFIG2, 0x1d),
> +

Stray newline.

> +};
> +

> +static const struct qmp_phy_cfg x1e80100_qmp_gen4x8_pciephy_cfg = {
> + .lanes = 8,
> +
> + .offsets = &qmp_pcie_offsets_v6_30,
> + .tbls = {
> + .serdes = x1e80100_qmp_gen4x8_pcie_serdes_tbl,
> + .serdes_num = ARRAY_SIZE(x1e80100_qmp_gen4x8_pcie_serdes_tbl),
> + .rx = x1e80100_qmp_gen4x8_pcie_rx_tbl,
> + .rx_num = ARRAY_SIZE(x1e80100_qmp_gen4x8_pcie_rx_tbl),
> + .pcs = x1e80100_qmp_gen4x8_pcie_pcs_tbl,
> + .pcs_num = ARRAY_SIZE(x1e80100_qmp_gen4x8_pcie_pcs_tbl),
> + .pcs_misc = x1e80100_qmp_gen4x8_pcie_pcs_misc_tbl,
> + .pcs_misc_num = ARRAY_SIZE(x1e80100_qmp_gen4x8_pcie_pcs_misc_tbl),
> + .ln_shrd = x1e80100_qmp_gen4x8_pcie_ln_shrd_tbl,
> + .ln_shrd_num = ARRAY_SIZE(x1e80100_qmp_gen4x8_pcie_ln_shrd_tbl),
> + .txz = x1e80100_qmp_gen4x8_pcie_txz_tbl,
> + .txz_num = ARRAY_SIZE(x1e80100_qmp_gen4x8_pcie_txz_tbl),
> + .rxz = x1e80100_qmp_gen4x8_pcie_rxz_tbl,
> + .rxz_num = ARRAY_SIZE(x1e80100_qmp_gen4x8_pcie_rxz_tbl),

Try follow the order of the other SoCs here as well (e.g. use the order
defined in struct qmp_phy_cfg_tbls).

> + },

> static void qmp_pcie_init_port_b(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tbls *tbls)
> {
> const struct qmp_phy_cfg *cfg = qmp->cfg;
> @@ -3751,6 +3953,9 @@ static void qmp_pcie_init_registers(struct qmp_pcie *qmp, const struct qmp_phy_c
>
> qmp_configure(qmp->dev, serdes, tbls->serdes, tbls->serdes_num);

If your comment in the commit message implies that txz/rxz must be
programmed before tx/rx then you need to add a comment here as well.

> + qmp_configure(qmp->dev, qmp->txz, tbls->txz, tbls->txz_num);
> + qmp_configure(qmp->dev, qmp->rxz, tbls->rxz, tbls->rxz_num);
> +
> qmp_configure_lane(qmp->dev, tx, tbls->tx, tbls->tx_num, 1);
> qmp_configure_lane(qmp->dev, rx, tbls->rx, tbls->rx_num, 1);

Johan