| 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: <Y8apUXpfAyaMMmH/@linaro.org>
Date: Tue, 17 Jan 2023 15:57:37 +0200
From: Abel Vesa <abel.vesa@...aro.org>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: Andy Gross <agross@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konrad.dybcio@...aro.org>,
"vkoul@...nel.org" <vkoul@...nel.org>,
Kishon Vijay Abraham I <kishon@...nel.org>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
devicetree@...r.kernel.org, linux-arm-msm@...r.kernel.org,
linux-phy@...ts.infradead.org,
Neil Armstrong <neil.armstrong@...aro.org>
Subject: Re: [PATCH 07/10] phy: qualcomm: qmp-pcie: Add support for SM8550
g3x2 and g4x2 PCIEs
On 23-01-01 22:15:55, Dmitry Baryshkov wrote:
> On 16/11/2022 14:01, Abel Vesa wrote:
> > Add the SM8550 both g4 and g3 configurations. In addition, there is a
> > new "lane shared" table that needs to be configured for g4, along with
> > the No-CSR list of resets.
> >
> > Co-developed-by: Neil Armstrong <neil.armstrong@...aro.org>
> > Signed-off-by: Neil Armstrong <neil.armstrong@...aro.org>
> > Signed-off-by: Abel Vesa <abel.vesa@...aro.org>
> > ---
> > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 354 +++++++++++++++++++++++
> > 1 file changed, 354 insertions(+)
> >
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index 47cccc4b35b2..87c7c20dfc8d 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>
> [skipped tables]
>
> > @@ -1473,6 +1701,8 @@ struct qmp_pcie_offsets {
> > struct qmp_phy_cfg_tbls {
> > const struct qmp_phy_init_tbl *serdes;
> > int serdes_num;
> > + const struct qmp_phy_init_tbl *ln_shrd_serdes;
> > + int ln_shrd_serdes_num;
> > const struct qmp_phy_init_tbl *tx;
> > int tx_num;
> > const struct qmp_phy_init_tbl *rx;
> > @@ -1510,6 +1740,9 @@ struct qmp_phy_cfg {
> > /* resets to be requested */
> > const char * const *reset_list;
> > int num_resets;
> > + /* no CSR resets to be requested */
> > + const char * const *nocsr_reset_list;
> > + int num_nocsr_resets;
>
> Is there any difference between 'no CSR' resets and the plain ones? Can we
> handle them in a single array instead?
Yes, on power on, only the 'No CSR' are necessary to be deasserted.
So we need to differentiate between 'No CSR' and the rest.
>
> > /* regulators to be requested */
> > const char * const *vreg_list;
> > int num_vregs;
> > @@ -1523,6 +1756,9 @@ struct qmp_phy_cfg {
> > bool skip_start_delay;
> > + /* true, if PHY has lane shared serdes table */
> > + bool has_ln_shrd_serdes_tbl;
>
> s/shrd/shared/g ? I think it's easier to read and to understand.
Sure. Will do.
>
> > +
> > /* QMP PHY pipe clock interface rate */
> > unsigned long pipe_clock_rate;
> > };
> > @@ -1534,6 +1770,7 @@ struct qmp_pcie {
> > bool tcsr_4ln_config;
> > void __iomem *serdes;
> > + void __iomem *ln_shrd_serdes;
> > void __iomem *pcs;
> > void __iomem *pcs_misc;
> > void __iomem *tx;
> > @@ -1548,6 +1785,7 @@ struct qmp_pcie {
> > int num_pipe_clks;
> > struct reset_control_bulk_data *resets;
> > + struct reset_control_bulk_data *nocsr_resets;
> > struct regulator_bulk_data *vregs;
> > struct phy *phy;
> > @@ -1595,11 +1833,19 @@ static const char * const sdm845_pciephy_clk_l[] = {
> > "aux", "cfg_ahb", "ref", "refgen",
> > };
> > +static const char * const sm8550_pciephy_clk_l[] = {
> > + "aux", "aux_phy", "cfg_ahb", "ref", "refgen",
> > +};
> > +
> > /* list of regulators */
> > static const char * const qmp_phy_vreg_l[] = {
> > "vdda-phy", "vdda-pll",
> > };
> > +static const char * const sm8550_qmp_phy_vreg_l[] = {
> > + "vdda-phy", "vdda-pll", "vdda-qref",
> > +};
> > +
> > /* list of resets */
> > static const char * const ipq8074_pciephy_reset_l[] = {
> > "phy", "common",
> > @@ -1609,6 +1855,10 @@ static const char * const sdm845_pciephy_reset_l[] = {
> > "phy",
> > };
> > +static const char * const sm8550_pciephy_nocsr_reset_l[] = {
> > + "pcie_1_nocsr_com_phy_reset",
> > +};
> > +
> > static const struct qmp_pcie_offsets qmp_pcie_offsets_v5 = {
> > .serdes = 0,
> > .pcs = 0x0200,
> > @@ -2084,6 +2334,65 @@ static const struct qmp_phy_cfg sm8450_qmp_gen4x2_pciephy_cfg = {
> > .phy_status = PHYSTATUS_4_20,
> > };
> > +static const struct qmp_phy_cfg sm8550_qmp_gen3x2_pciephy_cfg = {
> > + .lanes = 2,
> > +
> > + .tbls = {
> > + .serdes = sm8550_qmp_gen3x2_pcie_serdes_tbl,
> > + .serdes_num = ARRAY_SIZE(sm8550_qmp_gen3x2_pcie_serdes_tbl),
> > + .tx = sm8550_qmp_gen3x2_pcie_tx_tbl,
> > + .tx_num = ARRAY_SIZE(sm8550_qmp_gen3x2_pcie_tx_tbl),
> > + .rx = sm8550_qmp_gen3x2_pcie_rx_tbl,
> > + .rx_num = ARRAY_SIZE(sm8550_qmp_gen3x2_pcie_rx_tbl),
> > + .pcs = sm8550_qmp_gen3x2_pcie_pcs_tbl,
> > + .pcs_num = ARRAY_SIZE(sm8550_qmp_gen3x2_pcie_pcs_tbl),
> > + .pcs_misc = sm8550_qmp_gen3x2_pcie_pcs_misc_tbl,
> > + .pcs_misc_num = ARRAY_SIZE(sm8550_qmp_gen3x2_pcie_pcs_misc_tbl),
> > + },
> > + .clk_list = sdm845_pciephy_clk_l,
> > + .num_clks = ARRAY_SIZE(sdm845_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 sm8550_qmp_gen4x2_pciephy_cfg = {
> > + .lanes = 2,
> > +
> > + .tbls = {
> > + .serdes = sm8550_qmp_gen4x2_pcie_serdes_tbl,
> > + .serdes_num = ARRAY_SIZE(sm8550_qmp_gen4x2_pcie_serdes_tbl),
> > + .ln_shrd_serdes = sm8550_qmp_gen4x2_pcie_serdes_ln_shrd_tbl,
> > + .ln_shrd_serdes_num = ARRAY_SIZE(sm8550_qmp_gen4x2_pcie_serdes_ln_shrd_tbl),
> > + .tx = sm8550_qmp_gen4x2_pcie_tx_tbl,
> > + .tx_num = ARRAY_SIZE(sm8550_qmp_gen4x2_pcie_tx_tbl),
> > + .rx = sm8550_qmp_gen4x2_pcie_rx_tbl,
> > + .rx_num = ARRAY_SIZE(sm8550_qmp_gen4x2_pcie_rx_tbl),
> > + .pcs = sm8550_qmp_gen4x2_pcie_pcs_tbl,
> > + .pcs_num = ARRAY_SIZE(sm8550_qmp_gen4x2_pcie_pcs_tbl),
> > + .pcs_misc = sm8550_qmp_gen4x2_pcie_pcs_misc_tbl,
> > + .pcs_misc_num = ARRAY_SIZE(sm8550_qmp_gen4x2_pcie_pcs_misc_tbl),
> > + },
> > + .clk_list = sm8550_pciephy_clk_l,
> > + .num_clks = ARRAY_SIZE(sm8550_pciephy_clk_l),
> > + .reset_list = sdm845_pciephy_reset_l,
> > + .num_resets = ARRAY_SIZE(sdm845_pciephy_reset_l),
> > + .nocsr_reset_list = sm8550_pciephy_nocsr_reset_l,
> > + .num_nocsr_resets = ARRAY_SIZE(sm8550_pciephy_nocsr_reset_l),
> > + .vreg_list = sm8550_qmp_phy_vreg_l,
> > + .num_vregs = ARRAY_SIZE(sm8550_qmp_phy_vreg_l),
> > + .regs = sm8250_pcie_regs_layout,
> > +
> > + .has_ln_shrd_serdes_tbl = true,
> > + .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL,
> > + .phy_status = PHYSTATUS_4_20,
> > +};
> > +
> > static void qmp_pcie_configure_lane(void __iomem *base,
> > const struct qmp_phy_init_tbl tbl[],
> > int num,
> > @@ -2132,6 +2441,7 @@ static void qmp_pcie_init_registers(struct qmp_pcie *qmp, const struct qmp_phy_c
> > {
> > const struct qmp_phy_cfg *cfg = qmp->cfg;
> > void __iomem *serdes = qmp->serdes;
> > + void __iomem *ln_shrd_serdes = qmp->ln_shrd_serdes;
> > void __iomem *tx = qmp->tx;
> > void __iomem *rx = qmp->rx;
> > void __iomem *tx2 = qmp->tx2;
> > @@ -2159,6 +2469,10 @@ static void qmp_pcie_init_registers(struct qmp_pcie *qmp, const struct qmp_phy_c
> > qmp_pcie_configure(serdes, cfg->serdes_4ln_tbl, cfg->serdes_4ln_num);
> > qmp_pcie_init_port_b(qmp, tbls);
> > }
> > +
> > + if (cfg->has_ln_shrd_serdes_tbl)
> > + qmp_pcie_configure(ln_shrd_serdes, tbls->ln_shrd_serdes,
> > + tbls->ln_shrd_serdes_num);
> > }
> > static int qmp_pcie_init(struct phy *phy)
> > @@ -2179,6 +2493,14 @@ static int qmp_pcie_init(struct phy *phy)
> > goto err_disable_regulators;
> > }
> > + if (qmp->nocsr_resets) {
> > + ret = reset_control_bulk_assert(cfg->num_nocsr_resets, qmp->nocsr_resets);
> > + if (ret) {
> > + dev_err(qmp->dev, "no-csr reset assert failed\n");
> > + goto err_disable_regulators;
> > + }
> > + }
> > +
> > usleep_range(200, 300);
> > ret = reset_control_bulk_deassert(cfg->num_resets, qmp->resets);
> > @@ -2240,6 +2562,14 @@ static int qmp_pcie_power_on(struct phy *phy)
> > if (ret)
> > return ret;
> > + if (qmp->nocsr_resets) {
> > + ret = reset_control_bulk_deassert(cfg->num_nocsr_resets, qmp->nocsr_resets);
> > + if (ret) {
> > + dev_err(qmp->dev, "no-csr reset deassert failed\n");
> > + goto err_disable_pipe_clk;
> > + }
> > + }
> > +
> > /* Pull PHY out of reset state */
> > qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
> > @@ -2373,6 +2703,21 @@ static int qmp_pcie_reset_init(struct qmp_pcie *qmp)
> > if (ret)
> > return dev_err_probe(dev, ret, "failed to get resets\n");
> > + if (cfg->nocsr_reset_list) {
> > + qmp->nocsr_resets = devm_kcalloc(dev, cfg->num_nocsr_resets,
> > + sizeof(*qmp->nocsr_resets), GFP_KERNEL);
> > + if (!qmp->nocsr_resets)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < cfg->num_nocsr_resets; i++)
> > + qmp->nocsr_resets[i].id = cfg->nocsr_reset_list[i];
> > +
> > + ret = devm_reset_control_bulk_get_exclusive(dev, cfg->num_nocsr_resets,
> > + qmp->nocsr_resets);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to get no CSR resets\n");
> > + }
> > +
> > return 0;
> > }
> > @@ -2502,6 +2847,9 @@ static int qmp_pcie_parse_dt_legacy(struct qmp_pcie *qmp, struct device_node *np
> > return PTR_ERR(qmp->rx2);
> > qmp->pcs_misc = devm_of_iomap(dev, np, 5, NULL);
> > +
> > + if (cfg->has_ln_shrd_serdes_tbl)
> > + qmp->ln_shrd_serdes = devm_of_iomap(dev, np, 6, NULL);
>
> I think we also need to check the returned value. Also, I think we can drop
> the conditional check here. we don't have to validate the DT, so if the reg
> is present in DT, then it's present. If not, it's not required.
Yeah, but I was trying to make sure that if there is a new type of reg
for some upcomming SoC, we don't take that as ln_shrd. That's why the check
was added.
I'll add the returned value check though.
>
> > } else {
> > qmp->pcs_misc = devm_of_iomap(dev, np, 3, NULL);
> > }
> > @@ -2729,6 +3077,12 @@ static const struct of_device_id qmp_pcie_of_match_table[] = {
> > }, {
> > .compatible = "qcom,sm8450-qmp-gen4x2-pcie-phy",
> > .data = &sm8450_qmp_gen4x2_pciephy_cfg,
> > + }, {
> > + .compatible = "qcom,sm8550-qmp-gen3x2-pcie-phy",
> > + .data = &sm8550_qmp_gen3x2_pciephy_cfg,
> > + }, {
> > + .compatible = "qcom,sm8550-qmp-gen4x2-pcie-phy",
> > + .data = &sm8550_qmp_gen4x2_pciephy_cfg,
> > },
> > { },
> > };
>
> --
> With best wishes
> Dmitry
>
Powered by blists - more mailing lists