[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 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?

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