| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2902e7e8-eddf-149c-06fd-86b85d8af326@linaro.org>
Date: Thu, 20 Oct 2022 06:43:47 +0300
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Johan Hovold <johan+linaro@...nel.org>,
Vinod Koul <vkoul@...nel.org>
Cc: Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 15/15] phy: qcom-qmp-pcie: add support for sc8280xp
4-lane PHYs
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@...nel.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
Powered by blists - more mailing lists