[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 at 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,
>  };