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-...@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