[v2,15/15] phy: qcom-qmp-pcie: add support for sc8280xp 4-lane PHYs
Commit Message
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(+)
Comments
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,
On Thu, Oct 20, 2022 at 06:43:47AM +0300, Dmitry Baryshkov wrote:
> 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?
No, there is just one serdes register update needed when in 4-lane mode,
besides programming tx/rx for the second port. Adding a third struct
qmp_phy_cfg_tables for this seems like overkill and would lead to a more
convoluted implementation of the programming sequence.
And you can't add it two one of the existing ones, as your comment seems
to suggest.
The gen3x4 PHYs can be in either 4-lane or 2-lane mode depending on the
TCSR configuration. Port A is programmed identically in both cases
except for this serdes register, and in 4-lane mode tx/rx also needs
to be programmed for port B.
> > +
> > /* 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.
I'm afraid those names do not make much sense. This TCSR register
controls whether the PHY is in 4-lane mode (instead of 2-lane mode).
> >
> > 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 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.
No, the tx and tx registers of the second port should be programmed
identically to that of the first port.
> > +}
> > +
> > 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?
No, I don't think that would be an improvement and would obscure the
fact that we're programming two otherwise identical ports (e.g. tx1 and
tx2 of port B is used for the third and fourth lanes).
Note that the above conditional encodes the difference in programming
sequence between 4-lane and 2-lane mode I described above (one serdes
register difference + tx/rx of port b).
Johan
On 20/10/2022 09:43, Johan Hovold wrote:
> On Thu, Oct 20, 2022 at 06:43:47AM +0300, Dmitry Baryshkov wrote:
>> 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?
>
> No, there is just one serdes register update needed when in 4-lane mode,
> besides programming tx/rx for the second port. Adding a third struct
> qmp_phy_cfg_tables for this seems like overkill and would lead to a more
> convoluted implementation of the programming sequence.
Ack. Let's have it this way and change it later if the need arises.
>
> And you can't add it two one of the existing ones, as your comment seems
> to suggest.
Please excuse me if I didn't write it clear enough. I suggested adding
another cfg_tables, as you correctly commented above.
>
> The gen3x4 PHYs can be in either 4-lane or 2-lane mode depending on the
> TCSR configuration. Port A is programmed identically in both cases
> except for this serdes register, and in 4-lane mode tx/rx also needs
> to be programmed for port B.
>
>>> +
>>> /* 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.
>
> I'm afraid those names do not make much sense. This TCSR register
> controls whether the PHY is in 4-lane mode (instead of 2-lane mode).
Well, we just need the info that it's 4-lane. It doesn't really matter
if this information comes from TCSR, DT or e.g. fuses. I'd say that TCSR
is a platform detail. Thus I'm suggesting a more generic name.
>
>>>
>>> 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 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.
>
> No, the tx and tx registers of the second port should be programmed
> identically to that of the first port.
As you would prefer. As a matter of fact, we do not have CFG_LANES in
the PCIe PHY. Thus I'm surprised that you didn't drop this. I think
CFG_LANES usage is limited to sm8250 USB and combo PHY configurations.
>
>>> +}
>>> +
>>> 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?
>
> No, I don't think that would be an improvement and would obscure the
> fact that we're programming two otherwise identical ports (e.g. tx1 and
> tx2 of port B is used for the third and fourth lanes).
>
> Note that the above conditional encodes the difference in programming
> sequence between 4-lane and 2-lane mode I described above (one serdes
> register difference + tx/rx of port b).
Either way looks fine to me. So, let's leave this in a way you've
implemented it.
On Thu, Oct 20, 2022 at 12:32:13PM +0300, Dmitry Baryshkov wrote:
> On 20/10/2022 09:43, Johan Hovold wrote:
> > On Thu, Oct 20, 2022 at 06:43:47AM +0300, Dmitry Baryshkov wrote:
> >> 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>
> > The gen3x4 PHYs can be in either 4-lane or 2-lane mode depending on the
> > TCSR configuration. Port A is programmed identically in both cases
> > except for this serdes register, and in 4-lane mode tx/rx also needs
> > to be programmed for port B.
> >
> >>> +
> >>> /* 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.
> >
> > I'm afraid those names do not make much sense. This TCSR register
> > controls whether the PHY is in 4-lane mode (instead of 2-lane mode).
>
> Well, we just need the info that it's 4-lane. It doesn't really matter
> if this information comes from TCSR, DT or e.g. fuses. I'd say that TCSR
> is a platform detail. Thus I'm suggesting a more generic name.
No, it's a specific configuration flag for this (and possibly coming
platforms) to control whether the two PHY ports are used as individual x2
PHYs or as a combined x4 PHY.
It's not just about number of lanes and can definitely not come from DT
or somewhere else as that TCSR bit drives a signal that's needed during
programming.
> >>> +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.
> >
> > No, the tx and tx registers of the second port should be programmed
> > identically to that of the first port.
>
> As you would prefer. As a matter of fact, we do not have CFG_LANES in
> the PCIe PHY. Thus I'm surprised that you didn't drop this. I think
> CFG_LANES usage is limited to sm8250 USB and combo PHY configurations.
It's actually also used by SC8280XP so we cannot drop it (see
sc8280xp_qmp_gen3x2_pcie_tx_tbl) here. Appears to be unused for UFS
currently, though.
Johan
@@ -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.
@@ -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;
+
/* 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;
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);
+}
+
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);
+ }
}
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,