[PATCH v2 0/5] phy: rockchip: snps-pcie3: Fix bifurcation and spelling

[PATCH v2 0/5] phy: rockchip: snps-pcie3: Fix bifurcation and spelling

[Sebastian Kropatsch]

Several fixes for RK3588's PCIe PHY from upstream Linux [1][2], which
uses a very similar driver.
Tested on a FriendlyElec CM3588 NAS which uses PCIe bifurcation in
1x1x1x1 mode to enable four M.2 NVMe sockets. With these fixes applied,
NVMe SSDs get properly recognized in U-Boot.

In addition, v2 introduces a new patch to fix combo PHY muxing for
RK3588. [3]

While at it, correct a few instances where "rockchip" was written as
"rochchip" in two Rockchip PHY files.

Cheers,
Sebastian
---

Changes v1 -> v2:

 - Add new patch "phy: rockchip: naneng-combphy: Introduce PHY-IDs to fix RK3588 muxing"
 - Collect "Reviewed-by: Kever Yang" tags for all previous patches
 - Link to v1: https://lore.kernel.org/u-boot/cover.1720988760.git.seb-dev@mail.de/T/

Links:
[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c?id=f8020dfb311d2b6cf657668792aaa5fa8863a7dd
[2] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c?id=55491a5fa163bf15158f34f3650b3985f25622b9
[3] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c?id=d16d4002fea69b6609b852dd8db1f5844c02fbe4

---
Sebastian Kropatsch (5):
  phy: rockchip: naneng-combphy: Fix "rockchip" spelling
  phy: rockchip: naneng-combphy: Introduce PHY-IDs to fix RK3588 muxing
  phy: rockchip: snps-pcie3: Fix "rockchip" spelling
  phy: rockchip: snps-pcie3: Fix bifurcation for RK3588
  phy: rockchip: snps-pcie3: Fix clearing PHP_GRF_PCIESEL_CON bits

 .../rockchip/phy-rockchip-naneng-combphy.c    | 53 +++++++++++++++++--
 .../phy/rockchip/phy-rockchip-snps-pcie3.c    | 42 +++++++--------
 2 files changed, 67 insertions(+), 28 deletions(-)

-- 
2.43.0

[PATCH v2 1/5] phy: rockchip: naneng-combphy: Fix "rockchip" spelling

[Sebastian Kropatsch]

Replace "rochchip" by "rockchip" in two instances.

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Signed-off-by: Sebastian Kropatsch <seb-dev@mail.de>
---
 drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index 3ad339bccc..1b85cbcce8 100644
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
@@ -225,7 +225,7 @@ static int rockchip_combphy_xlate(struct phy *phy, struct ofnode_phandle_args *a
 	return 0;
 }
 
-static const struct phy_ops rochchip_combphy_ops = {
+static const struct phy_ops rockchip_combphy_ops = {
 	.init = rockchip_combphy_init,
 	.exit = rockchip_combphy_exit,
 	.of_xlate = rockchip_combphy_xlate,
@@ -535,7 +535,7 @@ U_BOOT_DRIVER(rockchip_naneng_combphy) = {
 	.name		= "naneng-combphy",
 	.id		= UCLASS_PHY,
 	.of_match	= rockchip_combphy_ids,
-	.ops		= &rochchip_combphy_ops,
+	.ops		= &rockchip_combphy_ops,
 	.probe		= rockchip_combphy_probe,
 	.priv_auto	= sizeof(struct rockchip_combphy_priv),
 };
-- 
2.43.0

[PATCH v2 2/5] phy: rockchip: naneng-combphy: Introduce PHY-IDs to fix RK3588 muxing

[Sebastian Kropatsch]

Fix multiplex configuration for PCIe1L0 and PCIe1L1 in PCIESEL_CON for
RK3588 to correctly select between Combo PHYs and PCIe3 PHY.
Currently, the code incorrectly muxes both ports to Combo PHYs,
interfering with PCIe3 PHY settings.
Introduce PHY identifiers to identify the correct Combo PHY and set
the necessary bits accordingly.

This fix is adapted from the upstream Linux commit by Sebastian Reichel:
d16d4002fea6 ("phy: rockchip: naneng-combphy: Fix mux on rk3588")

Fixes: b37260bca1aa ("phy: rockchip: naneng-combphy: Use signal from comb PHY on RK3588")
Signed-off-by: Sebastian Kropatsch <seb-dev@mail.de>
---
 .../rockchip/phy-rockchip-naneng-combphy.c    | 49 ++++++++++++++++++-
 1 file changed, 47 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index 1b85cbcce8..7d61913af1 100644
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
@@ -67,12 +67,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 {
 	u32 mode;
+	int id;
 	void __iomem *mmio;
 	struct udevice *dev;
 	struct regmap *pipe_grf;
@@ -270,6 +273,8 @@ static int rockchip_combphy_probe(struct udevice *udev)
 {
 	struct rockchip_combphy_priv *priv = dev_get_priv(udev);
 	const struct rockchip_combphy_cfg *phy_cfg;
+	fdt_addr_t addr;
+	int id;
 
 	priv->mmio = (void __iomem *)dev_read_addr(udev);
 	if (IS_ERR(priv->mmio))
@@ -281,6 +286,26 @@ static int rockchip_combphy_probe(struct udevice *udev)
 		return -EINVAL;
 	}
 
+	addr = dev_read_addr(udev);
+	if (addr == FDT_ADDR_T_NONE) {
+		dev_err(udev, "No valid device address found\n");
+		return -EINVAL;
+	}
+
+	/* Find the phy-id based on the device's I/O-address */
+	priv->id = -ENODEV;
+	for (id = 0; id < phy_cfg->num_phys; id++) {
+		if (addr == phy_cfg->phy_ids[id]) {
+			priv->id = id;
+			break;
+		}
+	}
+
+	if (priv->id == -ENODEV) {
+		dev_err(udev, "Failed to find PHY ID\n");
+		return -ENODEV;
+	}
+
 	priv->dev = udev;
 	priv->mode = PHY_TYPE_SATA;
 	priv->cfg = phy_cfg;
@@ -421,6 +446,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,
 };
@@ -436,8 +467,16 @@ static int rk3588_combphy_cfg(struct rockchip_combphy_priv *priv)
 		param_write(priv->phy_grf, &cfg->con1_for_pcie, true);
 		param_write(priv->phy_grf, &cfg->con2_for_pcie, true);
 		param_write(priv->phy_grf, &cfg->con3_for_pcie, true);
-		param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
-		param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
+		switch (priv->id) {
+		case 1:
+			printf("rk3588_combphy_cfg: priv->id was configured to 1");
+			param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
+			break;
+		case 2:
+			printf("rk3588_combphy_cfg: priv->id was configured to 2");
+			param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
+			break;
+		}
 		break;
 	case PHY_TYPE_USB3:
 		param_write(priv->phy_grf, &cfg->pipe_txcomp_sel, false);
@@ -515,6 +554,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

[PATCH v2 3/5] phy: rockchip: snps-pcie3: Fix "rockchip" spelling

[Sebastian Kropatsch]

Several identifiers use "rochchip" instead of "rockchip".
Fix this by replacing every instance of "rochchip" with "rockchip".

Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Signed-off-by: Sebastian Kropatsch <seb-dev@mail.de>
---
 drivers/phy/rockchip/phy-rockchip-snps-pcie3.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
index 2737bd81dd..1c94875aaa 100644
--- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
+++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
@@ -164,7 +164,7 @@ static const struct rockchip_p3phy_ops rk3588_ops = {
 	.phy_init = rockchip_p3phy_rk3588_init,
 };
 
-static int rochchip_p3phy_init(struct phy *phy)
+static int rockchip_p3phy_init(struct phy *phy)
 {
 	struct rockchip_p3phy_ops *ops =
 		(struct rockchip_p3phy_ops *)dev_get_driver_data(phy->dev);
@@ -185,7 +185,7 @@ static int rochchip_p3phy_init(struct phy *phy)
 	return ret;
 }
 
-static int rochchip_p3phy_exit(struct phy *phy)
+static int rockchip_p3phy_exit(struct phy *phy)
 {
 	struct rockchip_p3phy_priv *priv = dev_get_priv(phy->dev);
 
@@ -251,9 +251,9 @@ static int rockchip_p3phy_probe(struct udevice *dev)
 	return 0;
 }
 
-static struct phy_ops rochchip_p3phy_ops = {
-	.init = rochchip_p3phy_init,
-	.exit = rochchip_p3phy_exit,
+static struct phy_ops rockchip_p3phy_ops = {
+	.init = rockchip_p3phy_init,
+	.exit = rockchip_p3phy_exit,
 };
 
 static const struct udevice_id rockchip_p3phy_of_match[] = {
@@ -272,7 +272,7 @@ U_BOOT_DRIVER(rockchip_pcie3phy) = {
 	.name		= "rockchip_pcie3phy",
 	.id		= UCLASS_PHY,
 	.of_match	= rockchip_p3phy_of_match,
-	.ops		= &rochchip_p3phy_ops,
+	.ops		= &rockchip_p3phy_ops,
 	.probe		= rockchip_p3phy_probe,
 	.priv_auto	= sizeof(struct rockchip_p3phy_priv),
 };
-- 
2.43.0

[PATCH v2 4/5] phy: rockchip: snps-pcie3: Fix bifurcation for RK3588

[Sebastian Kropatsch]

Misconfigured `PHP_GRF_PCIESEL` values are causing bifurcation issues,
for example on the FriendlyElec CM3588 NAS board which uses bifurcation
on both PCIe PCIe ports (all four lanes) to enable four M.2 NVMe
sockets. Without this fix, NVMe devices do not get recognized.

Correct the `PHP_GRF_PCIESEL` register configuration and simplify the
bifurcation logic, enabling proper PCIe bifurcation based on the
data-lanes property.

This fix is adapted from the upstream Linux commit by Michal Tomek:
f8020dfb311d ("phy: rockchip-snps-pcie3: fix bifurcation on rk3588")

Fixes: 50e54e80679b ("phy: rockchip: snps-pcie3: Add support for RK3588")
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Signed-off-by: Sebastian Kropatsch <seb-dev@mail.de>
---
 .../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 1c94875aaa..fadb77c25c 100644
--- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
+++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
@@ -108,7 +108,7 @@ static int rockchip_p3phy_rk3588_init(struct phy *phy)
 {
 	struct rockchip_p3phy_priv *priv = dev_get_priv(phy->dev);
 	u32 reg = 0;
-	u8 mode = 0;
+	u8 mode = RK3588_LANE_AGGREGATION; /* Lane aggregation by default */
 	int ret;
 
 	/* Deassert PCIe PMA output clamp mode */
@@ -117,28 +117,20 @@ static int rockchip_p3phy_rk3588_init(struct phy *phy)
 
 	/* 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 */
-	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

[PATCH v2 5/5] phy: rockchip: snps-pcie3: Fix clearing PHP_GRF_PCIESEL_CON bits

[Sebastian Kropatsch]

The pcie1ln_sel bits for the RK3588 are getting set but not cleared due
to an incorrect write mask.
Use a newly introduced constant for the write mask to fix this.
Also introduce a GENMASK-based constant for PCIE30_PHY_MODE.

This fix is adapted from the upstream Linux commit by Sebastian Reichel:
55491a5fa163 ("phy: rockchip-snps-pcie3: fix clearing PHP_GRF_PCIESEL_CON bits")

Fixes: 50e54e80679b ("phy: rockchip: snps-pcie3: Add support for RK3588")
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>
Signed-off-by: Sebastian Kropatsch <seb-dev@mail.de>
---
 drivers/phy/rockchip/phy-rockchip-snps-pcie3.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
index fadb77c25c..62b42d1805 100644
--- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
+++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
@@ -36,6 +36,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_priv - RK DW PCIe PHY state
@@ -127,13 +129,13 @@ static int rockchip_p3phy_rk3588_init(struct phy *phy)
 
 	reg = mode;
 	regmap_write(priv->phy_grf, RK3588_PCIE3PHY_GRF_CMN_CON0,
-		     (0x7 << 16) | reg);
+		     RK3588_PCIE30_PHY_MODE_EN | reg);
 
 	/* Set pcie1ln_sel in PHP_GRF_PCIESEL_CON */
-	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_deassert(&priv->p30phy);
 	udelay(1);
-- 
2.43.0

Re: [PATCH v2 2/5] phy: rockchip: naneng-combphy: Introduce PHY-IDs to fix RK3588 muxing

[Kever Yang]

On 2024/7/17 04:42, Sebastian Kropatsch wrote:
> Fix multiplex configuration for PCIe1L0 and PCIe1L1 in PCIESEL_CON for
> RK3588 to correctly select between Combo PHYs and PCIe3 PHY.
> Currently, the code incorrectly muxes both ports to Combo PHYs,
> interfering with PCIe3 PHY settings.
> Introduce PHY identifiers to identify the correct Combo PHY and set
> the necessary bits accordingly.
>
> This fix is adapted from the upstream Linux commit by Sebastian Reichel:
> d16d4002fea6 ("phy: rockchip: naneng-combphy: Fix mux on rk3588")
>
> Fixes: b37260bca1aa ("phy: rockchip: naneng-combphy: Use signal from comb PHY on RK3588")
> Signed-off-by: Sebastian Kropatsch <seb-dev@mail.de>
Reviewed-by: Kever Yang <kever.yang@rock-chips.com>

Thanks,
- Kever
> ---
>   .../rockchip/phy-rockchip-naneng-combphy.c    | 49 ++++++++++++++++++-
>   1 file changed, 47 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index 1b85cbcce8..7d61913af1 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -67,12 +67,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 {
>   	u32 mode;
> +	int id;
>   	void __iomem *mmio;
>   	struct udevice *dev;
>   	struct regmap *pipe_grf;
> @@ -270,6 +273,8 @@ static int rockchip_combphy_probe(struct udevice *udev)
>   {
>   	struct rockchip_combphy_priv *priv = dev_get_priv(udev);
>   	const struct rockchip_combphy_cfg *phy_cfg;
> +	fdt_addr_t addr;
> +	int id;
>   
>   	priv->mmio = (void __iomem *)dev_read_addr(udev);
>   	if (IS_ERR(priv->mmio))
> @@ -281,6 +286,26 @@ static int rockchip_combphy_probe(struct udevice *udev)
>   		return -EINVAL;
>   	}
>   
> +	addr = dev_read_addr(udev);
> +	if (addr == FDT_ADDR_T_NONE) {
> +		dev_err(udev, "No valid device address found\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Find the phy-id based on the device's I/O-address */
> +	priv->id = -ENODEV;
> +	for (id = 0; id < phy_cfg->num_phys; id++) {
> +		if (addr == phy_cfg->phy_ids[id]) {
> +			priv->id = id;
> +			break;
> +		}
> +	}
> +
> +	if (priv->id == -ENODEV) {
> +		dev_err(udev, "Failed to find PHY ID\n");
> +		return -ENODEV;
> +	}
> +
>   	priv->dev = udev;
>   	priv->mode = PHY_TYPE_SATA;
>   	priv->cfg = phy_cfg;
> @@ -421,6 +446,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,
>   };
> @@ -436,8 +467,16 @@ static int rk3588_combphy_cfg(struct rockchip_combphy_priv *priv)
>   		param_write(priv->phy_grf, &cfg->con1_for_pcie, true);
>   		param_write(priv->phy_grf, &cfg->con2_for_pcie, true);
>   		param_write(priv->phy_grf, &cfg->con3_for_pcie, true);
> -		param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
> -		param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
> +		switch (priv->id) {
> +		case 1:
> +			printf("rk3588_combphy_cfg: priv->id was configured to 1");
> +			param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
> +			break;
> +		case 2:
> +			printf("rk3588_combphy_cfg: priv->id was configured to 2");
> +			param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
> +			break;
> +		}
>   		break;
>   	case PHY_TYPE_USB3:
>   		param_write(priv->phy_grf, &cfg->pipe_txcomp_sel, false);
> @@ -515,6 +554,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,
>   };

Re: [PATCH v2 2/5] phy: rockchip: naneng-combphy: Introduce PHY-IDs to fix RK3588 muxing

[Jonas Karlman]

Hi Sebastian,

On 2024-07-16 22:42, Sebastian Kropatsch wrote:
> Fix multiplex configuration for PCIe1L0 and PCIe1L1 in PCIESEL_CON for
> RK3588 to correctly select between Combo PHYs and PCIe3 PHY.
> Currently, the code incorrectly muxes both ports to Combo PHYs,
> interfering with PCIe3 PHY settings.
> Introduce PHY identifiers to identify the correct Combo PHY and set
> the necessary bits accordingly.
> 
> This fix is adapted from the upstream Linux commit by Sebastian Reichel:
> d16d4002fea6 ("phy: rockchip: naneng-combphy: Fix mux on rk3588")
> 
> Fixes: b37260bca1aa ("phy: rockchip: naneng-combphy: Use signal from comb PHY on RK3588")
> Signed-off-by: Sebastian Kropatsch <seb-dev@mail.de>
> ---
>  .../rockchip/phy-rockchip-naneng-combphy.c    | 49 ++++++++++++++++++-
>  1 file changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index 1b85cbcce8..7d61913af1 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -67,12 +67,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 {
>  	u32 mode;
> +	int id;
>  	void __iomem *mmio;
>  	struct udevice *dev;
>  	struct regmap *pipe_grf;
> @@ -270,6 +273,8 @@ static int rockchip_combphy_probe(struct udevice *udev)
>  {
>  	struct rockchip_combphy_priv *priv = dev_get_priv(udev);
>  	const struct rockchip_combphy_cfg *phy_cfg;
> +	fdt_addr_t addr;
> +	int id;
>  
>  	priv->mmio = (void __iomem *)dev_read_addr(udev);
>  	if (IS_ERR(priv->mmio))
> @@ -281,6 +286,26 @@ static int rockchip_combphy_probe(struct udevice *udev)
>  		return -EINVAL;
>  	}
>  
> +	addr = dev_read_addr(udev);

dev_read_addr(udev) is already called above, maybe we can use the
returned value from that call?

Or this could be changes to match other reg value matching done in other
rockchip phy drivers, e.g. something like:

	ret = ofnode_read_u32_index(dev_ofnode(udev), "reg", 0, &reg);
	if (ret) {
		dev_err(udev, "failed to read reg[0] property\n");
		return ret;
	}
	if (reg == 0 && dev_read_addr_cells(udev) == 2) {
		ret = ofnode_read_u32_index(dev_ofnode(udev), "reg", 1, &reg);
		if (ret) {
			dev_err(udev, "failed to read reg[1] property\n");
			return ret;
		}
	}

> +	if (addr == FDT_ADDR_T_NONE) {
> +		dev_err(udev, "No valid device address found\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Find the phy-id based on the device's I/O-address */
> +	priv->id = -ENODEV;
> +	for (id = 0; id < phy_cfg->num_phys; id++) {
> +		if (addr == phy_cfg->phy_ids[id]) {
> +			priv->id = id;
> +			break;
> +		}
> +	}
> +
> +	if (priv->id == -ENODEV) {
> +		dev_err(udev, "Failed to find PHY ID\n");
> +		return -ENODEV;
> +	}
> +
>  	priv->dev = udev;
>  	priv->mode = PHY_TYPE_SATA;
>  	priv->cfg = phy_cfg;
> @@ -421,6 +446,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,
>  };
> @@ -436,8 +467,16 @@ static int rk3588_combphy_cfg(struct rockchip_combphy_priv *priv)
>  		param_write(priv->phy_grf, &cfg->con1_for_pcie, true);
>  		param_write(priv->phy_grf, &cfg->con2_for_pcie, true);
>  		param_write(priv->phy_grf, &cfg->con3_for_pcie, true);
> -		param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
> -		param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
> +		switch (priv->id) {
> +		case 1:
> +			printf("rk3588_combphy_cfg: priv->id was configured to 1");

Please drop printf calls.

> +			param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
> +			break;
> +		case 2:
> +			printf("rk3588_combphy_cfg: priv->id was configured to 2");

Please drop printf calls.

Regards,
Jonas

> +			param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
> +			break;
> +		}
>  		break;
>  	case PHY_TYPE_USB3:
>  		param_write(priv->phy_grf, &cfg->pipe_txcomp_sel, false);
> @@ -515,6 +554,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,
>  };

Re: [PATCH v2 2/5] phy: rockchip: naneng-combphy: Introduce PHY-IDs to fix RK3588 muxing

[Sebastian Kropatsch]

Hi Jonas,

Am 21.07.2024 um 20:40 schrieb Jonas Karlman:
> Hi Sebastian,
> 
> On 2024-07-16 22:42, Sebastian Kropatsch wrote:
>> Fix multiplex configuration for PCIe1L0 and PCIe1L1 in PCIESEL_CON for
>> RK3588 to correctly select between Combo PHYs and PCIe3 PHY.
>> Currently, the code incorrectly muxes both ports to Combo PHYs,
>> interfering with PCIe3 PHY settings.
>> Introduce PHY identifiers to identify the correct Combo PHY and set
>> the necessary bits accordingly.
>>
>> This fix is adapted from the upstream Linux commit by Sebastian Reichel:
>> d16d4002fea6 ("phy: rockchip: naneng-combphy: Fix mux on rk3588")
>>
>> Fixes: b37260bca1aa ("phy: rockchip: naneng-combphy: Use signal from comb PHY on RK3588")
>> Signed-off-by: Sebastian Kropatsch <seb-dev@mail.de>
>> ---
>>   .../rockchip/phy-rockchip-naneng-combphy.c    | 49 ++++++++++++++++++-
>>   1 file changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>> index 1b85cbcce8..7d61913af1 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>> @@ -67,12 +67,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 {
>>   	u32 mode;
>> +	int id;
>>   	void __iomem *mmio;
>>   	struct udevice *dev;
>>   	struct regmap *pipe_grf;
>> @@ -270,6 +273,8 @@ static int rockchip_combphy_probe(struct udevice *udev)
>>   {
>>   	struct rockchip_combphy_priv *priv = dev_get_priv(udev);
>>   	const struct rockchip_combphy_cfg *phy_cfg;
>> +	fdt_addr_t addr;
>> +	int id;
>>   
>>   	priv->mmio = (void __iomem *)dev_read_addr(udev);
>>   	if (IS_ERR(priv->mmio))
>> @@ -281,6 +286,26 @@ static int rockchip_combphy_probe(struct udevice *udev)
>>   		return -EINVAL;
>>   	}
>>   
>> +	addr = dev_read_addr(udev);
> 
> dev_read_addr(udev) is already called above, maybe we can use the
> returned value from that call?

Yes that is true. While priv->mmio seems to be the base address of the
memory-mapped I/O region for the device, it seems to be more complex
than being simply of type fdt_addr_t. To my non-expert eye it seems
more error-prone, more unclear and less future-proof (what if __iomem
changes somehow?). Please correct me if I'm wrong.
I tried to keep it very close to the Linux implementation to make future
changes/imports from the Linux driver as easy as possible.

We could use the 'addr' variable in the priv->mmio definition though.

> Or this could be changes to match other reg value matching done in other
> rockchip phy drivers, e.g. something like:
> 
> 	ret = ofnode_read_u32_index(dev_ofnode(udev), "reg", 0, &reg);
> 	if (ret) {
> 		dev_err(udev, "failed to read reg[0] property\n");
> 		return ret;
> 	}
> 	if (reg == 0 && dev_read_addr_cells(udev) == 2) {
> 		ret = ofnode_read_u32_index(dev_ofnode(udev), "reg", 1, &reg);
> 		if (ret) {
> 			dev_err(udev, "failed to read reg[1] property\n");
> 			return ret;
> 		}
> 	}

This solution looks more complex to me. What would the advantage
of this solution be?

>> +	if (addr == FDT_ADDR_T_NONE) {
>> +		dev_err(udev, "No valid device address found\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	/* Find the phy-id based on the device's I/O-address */
>> +	priv->id = -ENODEV;
>> +	for (id = 0; id < phy_cfg->num_phys; id++) {
>> +		if (addr == phy_cfg->phy_ids[id]) {
>> +			priv->id = id;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (priv->id == -ENODEV) {
>> +		dev_err(udev, "Failed to find PHY ID\n");
>> +		return -ENODEV;
>> +	}
>> +
>>   	priv->dev = udev;
>>   	priv->mode = PHY_TYPE_SATA;
>>   	priv->cfg = phy_cfg;
>> @@ -421,6 +446,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,
>>   };
>> @@ -436,8 +467,16 @@ static int rk3588_combphy_cfg(struct rockchip_combphy_priv *priv)
>>   		param_write(priv->phy_grf, &cfg->con1_for_pcie, true);
>>   		param_write(priv->phy_grf, &cfg->con2_for_pcie, true);
>>   		param_write(priv->phy_grf, &cfg->con3_for_pcie, true);
>> -		param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
>> -		param_write(priv->pipe_grf, &cfg->pipe_pcie1l1_sel, true);
>> +		switch (priv->id) {
>> +		case 1:
>> +			printf("rk3588_combphy_cfg: priv->id was configured to 1");
> 
> Please drop printf calls.
> 
>> +			param_write(priv->pipe_grf, &cfg->pipe_pcie1l0_sel, true);
>> +			break;
>> +		case 2:
>> +			printf("rk3588_combphy_cfg: priv->id was configured to 2");
> 
> Please drop printf calls.

Yes, these are forgotten from debugging and shouldn't be there.

Cheers,
Sebastian