Messages in this thread Patch in this message | | | Date | Thu, 20 Oct 2022 06:43:47 +0300 | Subject | Re: [PATCH v2 15/15] phy: qcom-qmp-pcie: add support for sc8280xp 4-lane PHYs | From | Dmitry Baryshkov <> |
| |
On 19/10/2022 14:35, Johan Hovold wrote: > The PCIe2 and PCIe3 controllers and PHYs on SC8280XP can be used in > 4-lane mode or as separate controllers and PHYs in 2-lane mode (e.g. as > PCIe2A and PCIe2B). > > Add support for fetching the 4-lane configuration from the TCSR and > programming the lane registers of the second port when in 4-lane mode. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/phy/qualcomm/Kconfig | 1 + > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 118 +++++++++++++++++++++++ > 2 files changed, 119 insertions(+) > > diff --git a/drivers/phy/qualcomm/Kconfig b/drivers/phy/qualcomm/Kconfig > index 5c98850f5a36..eb9ddc685b38 100644 > --- a/drivers/phy/qualcomm/Kconfig > +++ b/drivers/phy/qualcomm/Kconfig > @@ -54,6 +54,7 @@ config PHY_QCOM_QMP > tristate "Qualcomm QMP PHY Driver" > depends on OF && COMMON_CLK && (ARCH_QCOM || COMPILE_TEST) > select GENERIC_PHY > + select MFD_SYSCON > help > Enable this to support the QMP PHY transceiver that is used > with controllers such as PCIe, UFS, and USB on Qualcomm chips. > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index ea5228bd9ecc..e5bce4810bb5 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > @@ -10,6 +10,7 @@ > #include <linux/io.h> > #include <linux/iopoll.h> > #include <linux/kernel.h> > +#include <linux/mfd/syscon.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > @@ -17,6 +18,7 @@ > #include <linux/phy/pcie.h> > #include <linux/phy/phy.h> > #include <linux/platform_device.h> > +#include <linux/regmap.h> > #include <linux/regulator/consumer.h> > #include <linux/reset.h> > #include <linux/slab.h> > @@ -886,6 +888,10 @@ static const struct qmp_phy_init_tbl sc8280xp_qmp_gen3x2_pcie_rc_serdes_tbl[] = > QMP_PHY_INIT_CFG(QSERDES_V5_COM_BIAS_EN_CLKBUFLR_EN, 0x14), > }; > > +static const struct qmp_phy_init_tbl sc8280xp_qmp_gen3x4_pcie_serdes_4ln_tbl[] = { > + QMP_PHY_INIT_CFG(QSERDES_V5_COM_BIAS_EN_CLKBUFLR_EN, 0x1c), > +}; > + > static const struct qmp_phy_init_tbl sc8280xp_qmp_gen3x1_pcie_tx_tbl[] = { > QMP_PHY_INIT_CFG(QSERDES_V5_TX_PI_QEC_CTRL, 0x20), > QMP_PHY_INIT_CFG(QSERDES_V5_TX_LANE_MODE_1, 0x75), > @@ -1491,6 +1497,9 @@ struct qmp_phy_cfg { > const struct qmp_phy_cfg_tables *tables_rc; > const struct qmp_phy_cfg_tables *tables_ep; > > + const struct qmp_phy_init_tbl *serdes_4ln_tbl; > + int serdes_4ln_num;
Would it make more sense to change this into the proper qmp_phy_cfg_tables entry and then use the existing API for programming the table?
> + > /* clock ids to be requested */ > const char * const *clk_list; > int num_clks; > @@ -1518,6 +1527,7 @@ struct qmp_pcie { > struct device *dev; > > const struct qmp_phy_cfg *cfg; > + bool tcsr_4ln_config;
As a matter of preference, this seems too specific. I'd rename it to split_config or split_4ln_config.
> > void __iomem *serdes; > void __iomem *pcs; > @@ -1527,6 +1537,8 @@ struct qmp_pcie { > void __iomem *tx2; > void __iomem *rx2; > > + void __iomem *port_b; > + > struct clk *pipe_clk; > struct clk *pipediv2_clk; > struct clk_bulk_data *clks; > @@ -1932,6 +1944,44 @@ static const struct qmp_phy_cfg sc8280xp_qmp_gen3x2_pciephy_cfg = { > .phy_status = PHYSTATUS, > }; > > +static const struct qmp_phy_cfg sc8280xp_qmp_gen3x4_pciephy_cfg = { > + .lanes = 4, > + > + .offsets = &qmp_pcie_offsets_v5, > + > + .tables = { > + .serdes = sc8280xp_qmp_pcie_serdes_tbl, > + .serdes_num = ARRAY_SIZE(sc8280xp_qmp_pcie_serdes_tbl), > + .tx = sc8280xp_qmp_gen3x2_pcie_tx_tbl, > + .tx_num = ARRAY_SIZE(sc8280xp_qmp_gen3x2_pcie_tx_tbl), > + .rx = sc8280xp_qmp_gen3x2_pcie_rx_tbl, > + .rx_num = ARRAY_SIZE(sc8280xp_qmp_gen3x2_pcie_rx_tbl), > + .pcs = sc8280xp_qmp_gen3x2_pcie_pcs_tbl, > + .pcs_num = ARRAY_SIZE(sc8280xp_qmp_gen3x2_pcie_pcs_tbl), > + .pcs_misc = sc8280xp_qmp_gen3x2_pcie_pcs_misc_tbl, > + .pcs_misc_num = ARRAY_SIZE(sc8280xp_qmp_gen3x2_pcie_pcs_misc_tbl), > + }, > + > + .tables_rc = &(const struct qmp_phy_cfg_tables) { > + .serdes = sc8280xp_qmp_gen3x2_pcie_rc_serdes_tbl, > + .serdes_num = ARRAY_SIZE(sc8280xp_qmp_gen3x2_pcie_rc_serdes_tbl), > + }, > + > + .serdes_4ln_tbl = sc8280xp_qmp_gen3x4_pcie_serdes_4ln_tbl, > + .serdes_4ln_num = ARRAY_SIZE(sc8280xp_qmp_gen3x4_pcie_serdes_4ln_tbl), > + > + .clk_list = sc8280xp_pciephy_clk_l, > + .num_clks = ARRAY_SIZE(sc8280xp_pciephy_clk_l), > + .reset_list = sdm845_pciephy_reset_l, > + .num_resets = ARRAY_SIZE(sdm845_pciephy_reset_l), > + .vreg_list = qmp_phy_vreg_l, > + .num_vregs = ARRAY_SIZE(qmp_phy_vreg_l), > + .regs = sm8250_pcie_regs_layout, > + > + .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, > + .phy_status = PHYSTATUS, > +}; > + > static const struct qmp_phy_cfg sdx55_qmp_pciephy_cfg = { > .lanes = 2, > > @@ -2054,6 +2104,24 @@ static void qmp_pcie_configure(void __iomem *base, > qmp_pcie_configure_lane(base, tbl, num, 0xff); > } > > +static void qmp_pcie_init_port_b(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls) > +{ > + const struct qmp_phy_cfg *cfg = qmp->cfg; > + const struct qmp_pcie_offsets *offs = cfg->offsets; > + void __iomem *tx3, *rx3, *tx4, *rx4; > + > + tx3 = qmp->port_b + offs->tx; > + rx3 = qmp->port_b + offs->rx; > + tx4 = qmp->port_b + offs->tx2; > + rx4 = qmp->port_b + offs->rx2; > + > + qmp_pcie_configure_lane(tx3, tbls->tx, tbls->tx_num, 1); > + qmp_pcie_configure_lane(rx3, tbls->rx, tbls->rx_num, 1); > + > + qmp_pcie_configure_lane(tx4, tbls->tx, tbls->tx_num, 2); > + qmp_pcie_configure_lane(rx4, tbls->rx, tbls->rx_num, 2);
I'd use BIT(2) and BIT(3) here. This would allow one to make a difference between programming first pair of lanes and second pair of lanes if necessary.
> +} > + > static void qmp_pcie_init_registers(struct qmp_pcie *qmp, const struct qmp_phy_cfg_tables *tbls) > { > const struct qmp_phy_cfg *cfg = qmp->cfg; > @@ -2080,6 +2148,11 @@ static void qmp_pcie_init_registers(struct qmp_pcie *qmp, const struct qmp_phy_c > > qmp_pcie_configure(pcs, tbls->pcs, tbls->pcs_num); > qmp_pcie_configure(pcs_misc, tbls->pcs_misc, tbls->pcs_misc_num); > + > + if (cfg->lanes >= 4 && qmp->tcsr_4ln_config) { > + qmp_pcie_configure(serdes, cfg->serdes_4ln_tbl, cfg->serdes_4ln_num); > + qmp_pcie_init_port_b(qmp, tbls); > + }
As you have been refactoring this piece of code, maybe it would make more sense to change qmp->tx/tx2 into an array of two elements? Then we can extend it to 4 in this patch, and just always write the whole array in a loop?
> } > > static int qmp_pcie_init(struct phy *phy) > @@ -2477,6 +2550,37 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np > return 0; > } > > +static int qmp_pcie_get_4ln_config(struct qmp_pcie *qmp) > +{ > + struct regmap *tcsr; > + unsigned int args[2]; > + int ret; > + > + tcsr = syscon_regmap_lookup_by_phandle_args(qmp->dev->of_node, > + "qcom,4ln-config-sel", > + ARRAY_SIZE(args), args); > + if (IS_ERR(tcsr)) { > + ret = PTR_ERR(tcsr); > + if (ret == -ENOENT) > + return 0; > + > + dev_err(qmp->dev, "failed to lookup syscon: %d\n", ret); > + return ret; > + } > + > + ret = regmap_test_bits(tcsr, args[0], BIT(args[1])); > + if (ret < 0) { > + dev_err(qmp->dev, "failed to read tcsr: %d\n", ret); > + return ret; > + } > + > + qmp->tcsr_4ln_config = ret; > + > + dev_dbg(qmp->dev, "4ln_config_sel = %d\n", qmp->tcsr_4ln_config); > + > + return 0; > +} > + > static int qmp_pcie_parse_dt(struct qmp_pcie *qmp) > { > struct platform_device *pdev = to_platform_device(qmp->dev); > @@ -2484,10 +2588,15 @@ static int qmp_pcie_parse_dt(struct qmp_pcie *qmp) > const struct qmp_pcie_offsets *offs = cfg->offsets; > struct device *dev = qmp->dev; > void __iomem *base; > + int ret; > > if (!offs) > return -EINVAL; > > + ret = qmp_pcie_get_4ln_config(qmp); > + if (ret) > + return ret; > + > base = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(base)) > return PTR_ERR(base); > @@ -2503,6 +2612,12 @@ static int qmp_pcie_parse_dt(struct qmp_pcie *qmp) > qmp->rx2 = base + offs->rx2; > } > > + if (qmp->cfg->lanes >= 4 && qmp->tcsr_4ln_config) { > + qmp->port_b = devm_platform_ioremap_resource(pdev, 1); > + if (IS_ERR(qmp->port_b)) > + return PTR_ERR(qmp->port_b); > + } > + > qmp->pipe_clk = devm_clk_get(dev, "pipe"); > if (IS_ERR(qmp->pipe_clk)) { > return dev_err_probe(dev, PTR_ERR(qmp->pipe_clk), > @@ -2610,6 +2725,9 @@ static const struct of_device_id qmp_pcie_of_match_table[] = { > }, { > .compatible = "qcom,sc8280xp-qmp-gen3x2-pcie-phy", > .data = &sc8280xp_qmp_gen3x2_pciephy_cfg, > + }, { > + .compatible = "qcom,sc8280xp-qmp-gen3x4-pcie-phy", > + .data = &sc8280xp_qmp_gen3x4_pciephy_cfg, > }, { > .compatible = "qcom,sdm845-qhp-pcie-phy", > .data = &sdm845_qhp_pciephy_cfg,
-- With best wishes Dmitry
| |