So far the RK3588 boards supported upstream do not make use of
bifurcation, so it went unnoticed that this feature is broken.
Michal Tomek tried getting a CM3588 running and noticed some
problems. These patches fix the bifurcation problems on CM3588
and also work fine on Rock 5B and EVB1.
---
Michal Tomek (1):
phy: rockchip-snps-pcie3: fix bifurcation on rk3588
Sebastian Reichel (2):
phy: rockchip-snps-pcie3: fix clearing PHP_GRF_PCIESEL_CON bits
phy: rockchip: naneng-combphy: Fix mux on rk3588
drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 36 ++++++++++++++++++++--
drivers/phy/rockchip/phy-rockchip-snps-pcie3.c | 31 ++++++++-----------
2 files changed, 46 insertions(+), 21 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240404-rk3588-pcie-bifurcation-fixes-471e96b94dea
Best regards,
--
Sebastian Reichel <[email protected]>
From: Sebastian Reichel <[email protected]>
Currently the PCIe v3 PHY driver only sets the pcie1ln_sel bits, but
does not clear them because of an incorrect write mask. This fixes up
the issue by using a newly introduced constant for the write mask.
While at it also introduces a proper GENMASK based constant for the
PCIE30_PHY_MODE.
Fixes: 2e9bffc4f713 ("phy: rockchip: Support PCIe v3")
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/phy/rockchip/phy-rockchip-snps-pcie3.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
index d5bcc9c42b28..9857ee45b89e 100644
--- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
+++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
@@ -40,6 +40,8 @@
#define RK3588_BIFURCATION_LANE_0_1 BIT(0)
#define RK3588_BIFURCATION_LANE_2_3 BIT(1)
#define RK3588_LANE_AGGREGATION BIT(2)
+#define RK3588_PCIE1LN_SEL_EN (GENMASK(1, 0) << 16)
+#define RK3588_PCIE30_PHY_MODE_EN (GENMASK(2, 0) << 16)
struct rockchip_p3phy_ops;
@@ -149,14 +151,15 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
}
reg = mode;
- regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0, (0x7<<16) | reg);
+ regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0,
+ RK3588_PCIE30_PHY_MODE_EN | reg);
/* Set pcie1ln_sel in PHP_GRF_PCIESEL_CON */
if (!IS_ERR(priv->pipe_grf)) {
- reg = mode & 3;
+ reg = mode & (RK3588_BIFURCATION_LANE_0_1 | RK3588_BIFURCATION_LANE_2_3);
if (reg)
regmap_write(priv->pipe_grf, PHP_GRF_PCIESEL_CON,
- (reg << 16) | reg);
+ RK3588_PCIE1LN_SEL_EN | reg);
}
reset_control_deassert(priv->p30phy);
--
2.43.0
From: Michal Tomek <[email protected]>
So far all RK3588 boards use fully aggregated PCIe. CM3588 is one
of the few boards using this feature and apparently it is broken.
The PHY offers the following mapping options:
port 0 lane 0 - always mapped to controller 0 (4L)
port 0 lane 1 - to controller 0 or 2 (1L0)
port 1 lane 0 - to controller 0 or 1 (2L)
port 1 lane 1 - to controller 0, 1 or 3 (1L1)
The data-lanes DT property maps these as follows:
0 = no controller (unsupported by the HW)
1 = 4L
2 = 2L
3 = 1L0
4 = 1L1
That allows the following configurations with first column being the
mainline data-lane mapping, second column being the downstream name,
third column being PCIE3PHY_GRF_CMN_CON0 and PHP_GRF_PCIESEL register
values and final column being the user visible lane setup:
<1 1 1 1> = AGGREG = [4 0] = x4 (aggregation)
<1 1 2 2> = NANBNB = [0 0] = x2 x2 (no bif.)
<1 3 2 2> = NANBBI = [1 1] = x2 x1x1 (bif. of port 0)
<1 1 2 4> = NABINB = [2 2] = x1x1 x2 (bif. of port 1)
<1 3 2 4> = NABIBI = [3 3] = x1x1 x1x1 (bif. of both ports)
The driver currently does not program PHP_GRF_PCIESEL correctly, which
is fixed by this patch. As a side-effect the new logic is much simpler
than the old logic.
Fixes: 2e9bffc4f713 ("phy: rockchip: Support PCIe v3")
Signed-off-by: Michal Tomek <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/phy/rockchip/phy-rockchip-snps-pcie3.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
index 121e5961ce11..d5bcc9c42b28 100644
--- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
+++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
@@ -132,7 +132,7 @@ static const struct rockchip_p3phy_ops rk3568_ops = {
static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
{
u32 reg = 0;
- u8 mode = 0;
+ u8 mode = RK3588_LANE_AGGREGATION; /* default */
int ret;
/* Deassert PCIe PMA output clamp mode */
@@ -140,28 +140,20 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
/* Set bifurcation if needed */
for (int i = 0; i < priv->num_lanes; i++) {
- if (!priv->lanes[i])
- mode |= (BIT(i) << 3);
-
if (priv->lanes[i] > 1)
- mode |= (BIT(i) >> 1);
- }
-
- if (!mode)
- reg = RK3588_LANE_AGGREGATION;
- else {
- if (mode & (BIT(0) | BIT(1)))
- reg |= RK3588_BIFURCATION_LANE_0_1;
-
- if (mode & (BIT(2) | BIT(3)))
- reg |= RK3588_BIFURCATION_LANE_2_3;
+ mode &= ~RK3588_LANE_AGGREGATION;
+ if (priv->lanes[i] == 3)
+ mode |= RK3588_BIFURCATION_LANE_0_1;
+ if (priv->lanes[i] == 4)
+ mode |= RK3588_BIFURCATION_LANE_2_3;
}
+ reg = mode;
regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0, (0x7<<16) | reg);
/* Set pcie1ln_sel in PHP_GRF_PCIESEL_CON */
if (!IS_ERR(priv->pipe_grf)) {
- reg = (mode & (BIT(6) | BIT(7))) >> 6;
+ reg = mode & 3;
if (reg)
regmap_write(priv->pipe_grf, PHP_GRF_PCIESEL_CON,
(reg << 16) | reg);
--
2.43.0
From: Sebastian Reichel <[email protected]>
The pcie1l0_sel and pcie1l1_sel bits in PCIESEL_CON configure the
mux for PCIe1L0 and PCIe1L1 to either the PIPE Combo PHYs or the
PCIe3 PHY. Thus this configuration interfers with the data-lanes
configuration done by the PCIe3 PHY.
RK3588 has three Combo PHYs. The first one has a dedicated PCIe
controller and is not affected by this. For the other two Combo
PHYs, there is one mux for each of them.
pcie1l0_sel selects if PCIe 1L0 is muxed to Combo PHY 1 when
bit is set to 0 or to the PCIe3 PHY when bit is set to 1.
pcie1l1_sel selects if PCIe 1L1 is muxed to Combo PHY 2 when
bit is set to 0 or to the PCIe3 PHY when bit is set to 1.
Currently the code always muxes 1L0 and 1L1 to the Combi PHYs
once one of them is being used in PCIe mode. This is obviously
wrong when at least one of the ports should be muxed to the
PCIe3 PHY.
Fix this by introducing Combo PHY identification and then only
setting up the required bit.
Fixes: a03c44277253 ("phy: rockchip: Add naneng combo phy support for RK3588")
Reported-by: Michal Tomek <[email protected]>
Signed-off-by: Sebastian Reichel <[email protected]>
---
drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 36 ++++++++++++++++++++--
1 file changed, 33 insertions(+), 3 deletions(-)
diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index 76b9cf417591..bf74e429ff46 100644
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
@@ -125,12 +125,15 @@ struct rockchip_combphy_grfcfg {
};
struct rockchip_combphy_cfg {
+ unsigned int num_phys;
+ unsigned int phy_ids[3];
const struct rockchip_combphy_grfcfg *grfcfg;
int (*combphy_cfg)(struct rockchip_combphy_priv *priv);
};
struct rockchip_combphy_priv {
u8 type;
+ int id;
void __iomem *mmio;
int num_clks;
struct clk_bulk_data *clks;
@@ -320,7 +323,7 @@ static int rockchip_combphy_probe(struct platform_device *pdev)
struct rockchip_combphy_priv *priv;
const struct rockchip_combphy_cfg *phy_cfg;
struct resource *res;
- int ret;
+ int ret, id;
phy_cfg = of_device_get_match_data(dev);
if (!phy_cfg) {
@@ -338,6 +341,15 @@ static int rockchip_combphy_probe(struct platform_device *pdev)
return ret;
}
+ /* find the phy-id from the io address */
+ priv->id = -ENODEV;
+ for (id = 0; id < phy_cfg->num_phys; id++) {
+ if (res->start == phy_cfg->phy_ids[id]) {
+ priv->id = id;
+ break;
+ }
+ }
+
priv->dev = dev;
priv->type = PHY_NONE;
priv->cfg = phy_cfg;
@@ -562,6 +574,12 @@ static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
};
static const struct rockchip_combphy_cfg rk3568_combphy_cfgs = {
+ .num_phys = 3,
+ .phy_ids = {
+ 0xfe820000,
+ 0xfe830000,
+ 0xfe840000,
+ },
.grfcfg = &rk3568_combphy_grfcfgs,
.combphy_cfg = rk3568_combphy_cfg,
};
@@ -578,8 +596,14 @@ static int rk3588_combphy_cfg(struct rockchip_combphy_priv *priv)
rockchip_combphy_param_write(priv->phy_grf, &cfg->con1_for_pcie, true);
rockchip_combphy_param_write(priv->phy_grf, &cfg->con2_for_pcie, true);
rockchip_combphy_param_write(priv->phy_grf, &cfg->con3_for_pcie, true);
- rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
- rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
+ switch (priv->id) {
+ case 1:
+ rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
+ break;
+ case 2:
+ rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
+ break;
+ }
break;
case PHY_TYPE_USB3:
/* Set SSC downward spread spectrum */
@@ -736,6 +760,12 @@ static const struct rockchip_combphy_grfcfg rk3588_combphy_grfcfgs = {
};
static const struct rockchip_combphy_cfg rk3588_combphy_cfgs = {
+ .num_phys = 3,
+ .phy_ids = {
+ 0xfee00000,
+ 0xfee10000,
+ 0xfee20000,
+ },
.grfcfg = &rk3588_combphy_grfcfgs,
.combphy_cfg = rk3588_combphy_cfg,
};
--
2.43.0
Am Donnerstag, 4. April 2024, 19:11:28 CEST schrieb Sebastian Reichel:
> From: Sebastian Reichel <[email protected]>
>
> The pcie1l0_sel and pcie1l1_sel bits in PCIESEL_CON configure the
> mux for PCIe1L0 and PCIe1L1 to either the PIPE Combo PHYs or the
> PCIe3 PHY. Thus this configuration interfers with the data-lanes
> configuration done by the PCIe3 PHY.
>
> RK3588 has three Combo PHYs. The first one has a dedicated PCIe
> controller and is not affected by this. For the other two Combo
> PHYs, there is one mux for each of them.
>
> pcie1l0_sel selects if PCIe 1L0 is muxed to Combo PHY 1 when
> bit is set to 0 or to the PCIe3 PHY when bit is set to 1.
>
> pcie1l1_sel selects if PCIe 1L1 is muxed to Combo PHY 2 when
> bit is set to 0 or to the PCIe3 PHY when bit is set to 1.
>
> Currently the code always muxes 1L0 and 1L1 to the Combi PHYs
> once one of them is being used in PCIe mode. This is obviously
> wrong when at least one of the ports should be muxed to the
> PCIe3 PHY.
>
> Fix this by introducing Combo PHY identification and then only
> setting up the required bit.
>
> Fixes: a03c44277253 ("phy: rockchip: Add naneng combo phy support for RK3588")
> Reported-by: Michal Tomek <[email protected]>
> Signed-off-by: Sebastian Reichel <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
> ---
> drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 36 ++++++++++++++++++++--
> 1 file changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index 76b9cf417591..bf74e429ff46 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -125,12 +125,15 @@ struct rockchip_combphy_grfcfg {
> };
>
> struct rockchip_combphy_cfg {
> + unsigned int num_phys;
> + unsigned int phy_ids[3];
> const struct rockchip_combphy_grfcfg *grfcfg;
> int (*combphy_cfg)(struct rockchip_combphy_priv *priv);
> };
>
> struct rockchip_combphy_priv {
> u8 type;
> + int id;
> void __iomem *mmio;
> int num_clks;
> struct clk_bulk_data *clks;
> @@ -320,7 +323,7 @@ static int rockchip_combphy_probe(struct platform_device *pdev)
> struct rockchip_combphy_priv *priv;
> const struct rockchip_combphy_cfg *phy_cfg;
> struct resource *res;
> - int ret;
> + int ret, id;
>
> phy_cfg = of_device_get_match_data(dev);
> if (!phy_cfg) {
> @@ -338,6 +341,15 @@ static int rockchip_combphy_probe(struct platform_device *pdev)
> return ret;
> }
>
> + /* find the phy-id from the io address */
> + priv->id = -ENODEV;
> + for (id = 0; id < phy_cfg->num_phys; id++) {
> + if (res->start == phy_cfg->phy_ids[id]) {
> + priv->id = id;
> + break;
> + }
> + }
> +
> priv->dev = dev;
> priv->type = PHY_NONE;
> priv->cfg = phy_cfg;
> @@ -562,6 +574,12 @@ static const struct rockchip_combphy_grfcfg rk3568_combphy_grfcfgs = {
> };
>
> static const struct rockchip_combphy_cfg rk3568_combphy_cfgs = {
> + .num_phys = 3,
> + .phy_ids = {
> + 0xfe820000,
> + 0xfe830000,
> + 0xfe840000,
> + },
> .grfcfg = &rk3568_combphy_grfcfgs,
> .combphy_cfg = rk3568_combphy_cfg,
> };
> @@ -578,8 +596,14 @@ static int rk3588_combphy_cfg(struct rockchip_combphy_priv *priv)
> rockchip_combphy_param_write(priv->phy_grf, &cfg->con1_for_pcie, true);
> rockchip_combphy_param_write(priv->phy_grf, &cfg->con2_for_pcie, true);
> rockchip_combphy_param_write(priv->phy_grf, &cfg->con3_for_pcie, true);
> - rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
> - rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
> + switch (priv->id) {
> + case 1:
> + rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
> + break;
> + case 2:
> + rockchip_combphy_param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
> + break;
> + }
> break;
> case PHY_TYPE_USB3:
> /* Set SSC downward spread spectrum */
> @@ -736,6 +760,12 @@ static const struct rockchip_combphy_grfcfg rk3588_combphy_grfcfgs = {
> };
>
> static const struct rockchip_combphy_cfg rk3588_combphy_cfgs = {
> + .num_phys = 3,
> + .phy_ids = {
> + 0xfee00000,
> + 0xfee10000,
> + 0xfee20000,
> + },
> .grfcfg = &rk3588_combphy_grfcfgs,
> .combphy_cfg = rk3588_combphy_cfg,
> };
>
>
Am Donnerstag, 4. April 2024, 19:11:27 CEST schrieb Sebastian Reichel:
> From: Sebastian Reichel <[email protected]>
>
> Currently the PCIe v3 PHY driver only sets the pcie1ln_sel bits, but
> does not clear them because of an incorrect write mask. This fixes up
> the issue by using a newly introduced constant for the write mask.
>
> While at it also introduces a proper GENMASK based constant for the
> PCIE30_PHY_MODE.
>
> Fixes: 2e9bffc4f713 ("phy: rockchip: Support PCIe v3")
> Signed-off-by: Sebastian Reichel <[email protected]>
after checking with the soc manual
Reviewed-by: Heiko Stuebner <[email protected]>
Am Donnerstag, 4. April 2024, 19:11:26 CEST schrieb Sebastian Reichel:
> From: Michal Tomek <[email protected]>
>
> So far all RK3588 boards use fully aggregated PCIe. CM3588 is one
> of the few boards using this feature and apparently it is broken.
>
> The PHY offers the following mapping options:
>
> port 0 lane 0 - always mapped to controller 0 (4L)
> port 0 lane 1 - to controller 0 or 2 (1L0)
> port 1 lane 0 - to controller 0 or 1 (2L)
> port 1 lane 1 - to controller 0, 1 or 3 (1L1)
>
> The data-lanes DT property maps these as follows:
>
> 0 = no controller (unsupported by the HW)
> 1 = 4L
> 2 = 2L
> 3 = 1L0
> 4 = 1L1
>
> That allows the following configurations with first column being the
> mainline data-lane mapping, second column being the downstream name,
> third column being PCIE3PHY_GRF_CMN_CON0 and PHP_GRF_PCIESEL register
> values and final column being the user visible lane setup:
>
> <1 1 1 1> = AGGREG = [4 0] = x4 (aggregation)
> <1 1 2 2> = NANBNB = [0 0] = x2 x2 (no bif.)
> <1 3 2 2> = NANBBI = [1 1] = x2 x1x1 (bif. of port 0)
> <1 1 2 4> = NABINB = [2 2] = x1x1 x2 (bif. of port 1)
> <1 3 2 4> = NABIBI = [3 3] = x1x1 x1x1 (bif. of both ports)
>
> The driver currently does not program PHP_GRF_PCIESEL correctly, which
> is fixed by this patch. As a side-effect the new logic is much simpler
> than the old logic.
>
> Fixes: 2e9bffc4f713 ("phy: rockchip: Support PCIe v3")
> Signed-off-by: Michal Tomek <[email protected]>
> Signed-off-by: Sebastian Reichel <[email protected]>
> ---
> drivers/phy/rockchip/phy-rockchip-snps-pcie3.c | 24 ++++++++----------------
> 1 file changed, 8 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> index 121e5961ce11..d5bcc9c42b28 100644
> --- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> +++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> @@ -132,7 +132,7 @@ static const struct rockchip_p3phy_ops rk3568_ops = {
> static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
> {
> u32 reg = 0;
> - u8 mode = 0;
> + u8 mode = RK3588_LANE_AGGREGATION; /* default */
> int ret;
>
> /* Deassert PCIe PMA output clamp mode */
> @@ -140,28 +140,20 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
>
> /* Set bifurcation if needed */
> for (int i = 0; i < priv->num_lanes; i++) {
> - if (!priv->lanes[i])
> - mode |= (BIT(i) << 3);
> -
> if (priv->lanes[i] > 1)
> - mode |= (BIT(i) >> 1);
> - }
> -
> - if (!mode)
> - reg = RK3588_LANE_AGGREGATION;
> - else {
> - if (mode & (BIT(0) | BIT(1)))
> - reg |= RK3588_BIFURCATION_LANE_0_1;
> -
> - if (mode & (BIT(2) | BIT(3)))
> - reg |= RK3588_BIFURCATION_LANE_2_3;
> + mode &= ~RK3588_LANE_AGGREGATION;
> + if (priv->lanes[i] == 3)
> + mode |= RK3588_BIFURCATION_LANE_0_1;
> + if (priv->lanes[i] == 4)
> + mode |= RK3588_BIFURCATION_LANE_2_3;
> }
>
> + reg = mode;
> regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0, (0x7<<16) | reg);
nit: instead of doing reg=mode, why not use mode directly?
i.e. (0x7<<16) | mode in the regmap_write call
other than that
Acked-by: Heiko Stuebner <[email protected]>
>
> /* Set pcie1ln_sel in PHP_GRF_PCIESEL_CON */
> if (!IS_ERR(priv->pipe_grf)) {
> - reg = (mode & (BIT(6) | BIT(7))) >> 6;
> + reg = mode & 3;
> if (reg)
> regmap_write(priv->pipe_grf, PHP_GRF_PCIESEL_CON,
> (reg << 16) | reg);
>
>
On Thu, 04 Apr 2024 19:11:25 +0200, Sebastian Reichel wrote:
> So far the RK3588 boards supported upstream do not make use of
> bifurcation, so it went unnoticed that this feature is broken.
> Michal Tomek tried getting a CM3588 running and noticed some
> problems. These patches fix the bifurcation problems on CM3588
> and also work fine on Rock 5B and EVB1.
>
Applied, thanks!
[1/3] phy: rockchip-snps-pcie3: fix bifurcation on rk3588
commit: f8020dfb311d2b6cf657668792aaa5fa8863a7dd
[2/3] phy: rockchip-snps-pcie3: fix clearing PHP_GRF_PCIESEL_CON bits
commit: 55491a5fa163bf15158f34f3650b3985f25622b9
[3/3] phy: rockchip: naneng-combphy: Fix mux on rk3588
commit: d16d4002fea69b6609b852dd8db1f5844c02fbe4
Best regards,
--
~Vinod