Re: [PATCH v2 15/15] phy: qcom-qmp-pcie: add support for sc8280xp 4-lane PHYs
From: Dmitry Baryshkov
Date: Thu Oct 20 2022 - 05:33:45 EST
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@xxxxxxxxxx>
---
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.
--
With best wishes
Dmitry