[PATCH 0/2] phy: phy-rockchip-samsung-hdptx: don't use of-alias

[PATCH 0/2] phy: phy-rockchip-samsung-hdptx: don't use of-alias

[Heiko Stuebner]

One nice comment addition and removal of of_alias usage for core
device functionality.

Heiko Stuebner (2):
  phy: phy-rockchip-samsung-hdptx: annotate regmap register-callback
  phy: phy-rockchip-samsung-hdptx: Don't use dt aliases to determine
    phy-id

 .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 64 +++++++++++++++----
 1 file changed, 51 insertions(+), 13 deletions(-)

-- 
2.45.2

[PATCH 1/2] phy: phy-rockchip-samsung-hdptx: annotate regmap register-callback

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@cherry.de>

The variant of the driver in the vendor-tree contained those handy
comments in the regmap register callback. Having the different ranges
describe what they are looks helpful.

Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
index 4ced1479e2dc..c5c64c209e96 100644
--- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
+++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
@@ -766,13 +766,13 @@ static struct tx_drv_ctrl tx_drv_ctrl_hbr2[4][4] = {
 static bool rk_hdptx_phy_is_rw_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
-	case 0x0000 ... 0x029c:
-	case 0x0400 ... 0x04a4:
-	case 0x0800 ... 0x08a4:
-	case 0x0c00 ... 0x0cb4:
-	case 0x1000 ... 0x10b4:
-	case 0x1400 ... 0x14b4:
-	case 0x1800 ... 0x18b4:
+	case 0x0000 ... 0x029c: /* CMN Register */
+	case 0x0400 ... 0x04a4: /* Sideband Register */
+	case 0x0800 ... 0x08a4: /* Lane Top Register */
+	case 0x0c00 ... 0x0cb4: /* Lane 0 Register */
+	case 0x1000 ... 0x10b4: /* Lane 1 Register */
+	case 0x1400 ... 0x14b4: /* Lane 2 Register */
+	case 0x1800 ... 0x18b4: /* Lane 3 Register */
 		return true;
 	}
 
-- 
2.45.2

Re: [PATCH 1/2] phy: phy-rockchip-samsung-hdptx: annotate regmap register-callback

[Cristian Ciocaltea]

On 12/6/24 12:34 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> The variant of the driver in the vendor-tree contained those handy
> comments in the regmap register callback. Having the different ranges
> describe what they are looks helpful.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> index 4ced1479e2dc..c5c64c209e96 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> @@ -766,13 +766,13 @@ static struct tx_drv_ctrl tx_drv_ctrl_hbr2[4][4] = {
>  static bool rk_hdptx_phy_is_rw_reg(struct device *dev, unsigned int reg)
>  {
>  	switch (reg) {
> -	case 0x0000 ... 0x029c:
> -	case 0x0400 ... 0x04a4:
> -	case 0x0800 ... 0x08a4:
> -	case 0x0c00 ... 0x0cb4:
> -	case 0x1000 ... 0x10b4:
> -	case 0x1400 ... 0x14b4:
> -	case 0x1800 ... 0x18b4:
> +	case 0x0000 ... 0x029c: /* CMN Register */
> +	case 0x0400 ... 0x04a4: /* Sideband Register */
> +	case 0x0800 ... 0x08a4: /* Lane Top Register */
> +	case 0x0c00 ... 0x0cb4: /* Lane 0 Register */
> +	case 0x1000 ... 0x10b4: /* Lane 1 Register */
> +	case 0x1400 ... 0x14b4: /* Lane 2 Register */
> +	case 0x1800 ... 0x18b4: /* Lane 3 Register */
>  		return true;
>  	}
>  

Nice finding!  The HDMI variant of the driver which I used as an
implementation reference (i.e. phy-rockchip-samsung-hdptx-hdmi.c)
doesn't provide these comments.

Reviewed-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

Re: [PATCH 1/2] phy: phy-rockchip-samsung-hdptx: annotate regmap register-callback

[Heiko Stübner]

Am Freitag, 6. Dezember 2024, 12:18:56 CET schrieb Cristian Ciocaltea:
> On 12/6/24 12:34 PM, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@cherry.de>
> > 
> > The variant of the driver in the vendor-tree contained those handy
> > comments in the regmap register callback. Having the different ranges
> > describe what they are looks helpful.
> > 
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> > ---
> >  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> > index 4ced1479e2dc..c5c64c209e96 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> > @@ -766,13 +766,13 @@ static struct tx_drv_ctrl tx_drv_ctrl_hbr2[4][4] = {
> >  static bool rk_hdptx_phy_is_rw_reg(struct device *dev, unsigned int reg)
> >  {
> >  	switch (reg) {
> > -	case 0x0000 ... 0x029c:
> > -	case 0x0400 ... 0x04a4:
> > -	case 0x0800 ... 0x08a4:
> > -	case 0x0c00 ... 0x0cb4:
> > -	case 0x1000 ... 0x10b4:
> > -	case 0x1400 ... 0x14b4:
> > -	case 0x1800 ... 0x18b4:
> > +	case 0x0000 ... 0x029c: /* CMN Register */
> > +	case 0x0400 ... 0x04a4: /* Sideband Register */
> > +	case 0x0800 ... 0x08a4: /* Lane Top Register */
> > +	case 0x0c00 ... 0x0cb4: /* Lane 0 Register */
> > +	case 0x1000 ... 0x10b4: /* Lane 1 Register */
> > +	case 0x1400 ... 0x14b4: /* Lane 2 Register */
> > +	case 0x1800 ... 0x18b4: /* Lane 3 Register */
> >  		return true;
> >  	}
> >  
> 
> Nice finding!  The HDMI variant of the driver which I used as an
> implementation reference (i.e. phy-rockchip-samsung-hdptx-hdmi.c)
> doesn't provide these comments.

yep, those comments were in the displayport variant of the phy-driver ;-) .
I don't really understand why the vendor tree contains 2 separate drivers
written by two different people, but at least it looks like we're getting
one driver in mainline, thanks to Damon's work :-)

Re: [PATCH 1/2] phy: phy-rockchip-samsung-hdptx: annotate regmap register-callback

[Cristian Ciocaltea]

On 12/6/24 7:36 PM, Heiko Stübner wrote:
> Am Freitag, 6. Dezember 2024, 12:18:56 CET schrieb Cristian Ciocaltea:
>> On 12/6/24 12:34 PM, Heiko Stuebner wrote:
>>> From: Heiko Stuebner <heiko.stuebner@cherry.de>
>>>
>>> The variant of the driver in the vendor-tree contained those handy
>>> comments in the regmap register callback. Having the different ranges
>>> describe what they are looks helpful.
>>>
>>> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
>>> ---
>>>  drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c | 14 +++++++-------
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>> index 4ced1479e2dc..c5c64c209e96 100644
>>> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
>>> @@ -766,13 +766,13 @@ static struct tx_drv_ctrl tx_drv_ctrl_hbr2[4][4] = {
>>>  static bool rk_hdptx_phy_is_rw_reg(struct device *dev, unsigned int reg)
>>>  {
>>>  	switch (reg) {
>>> -	case 0x0000 ... 0x029c:
>>> -	case 0x0400 ... 0x04a4:
>>> -	case 0x0800 ... 0x08a4:
>>> -	case 0x0c00 ... 0x0cb4:
>>> -	case 0x1000 ... 0x10b4:
>>> -	case 0x1400 ... 0x14b4:
>>> -	case 0x1800 ... 0x18b4:
>>> +	case 0x0000 ... 0x029c: /* CMN Register */
>>> +	case 0x0400 ... 0x04a4: /* Sideband Register */
>>> +	case 0x0800 ... 0x08a4: /* Lane Top Register */
>>> +	case 0x0c00 ... 0x0cb4: /* Lane 0 Register */
>>> +	case 0x1000 ... 0x10b4: /* Lane 1 Register */
>>> +	case 0x1400 ... 0x14b4: /* Lane 2 Register */
>>> +	case 0x1800 ... 0x18b4: /* Lane 3 Register */
>>>  		return true;
>>>  	}
>>>  
>>
>> Nice finding!  The HDMI variant of the driver which I used as an
>> implementation reference (i.e. phy-rockchip-samsung-hdptx-hdmi.c)
>> doesn't provide these comments.
> 
> yep, those comments were in the displayport variant of the phy-driver ;-) .
> I don't really understand why the vendor tree contains 2 separate drivers

Yeah, I asked myself the same question a while ago - I'd assume the
(initial) development happened in parallel, but the final sync/merge was
never done.

> written by two different people, but at least it looks like we're getting
> one driver in mainline, thanks to Damon's work :-)

Indeed, we're eventually getting there.. :-)

[PATCH 2/2] phy: phy-rockchip-samsung-hdptx: Don't use dt aliases to determine phy-id

[Heiko Stuebner]

From: Heiko Stuebner <heiko.stuebner@cherry.de>

The phy needs to know its identity in the system (phy0 or phy1 on rk3588)
for some actions and the driver currently contains code abusing of_alias
for that.

Devicetree aliases are always optional and should not be used for core
device functionality, so instead keep a list of phys on a soc in the
of_device_data and find the phy-id by comparing against the mapped
register-base.

Fixes: c4b09c562086 ("phy: phy-rockchip-samsung-hdptx: Add clock provider support")
Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
---
 .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 50 ++++++++++++++++---
 1 file changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
index c5c64c209e96..b137f8c4d157 100644
--- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
+++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
@@ -385,11 +385,22 @@ enum rk_hdptx_reset {
 	RST_MAX
 };
 
+#define MAX_HDPTX_PHY_NUM	2
+
+struct rk_hdptx_phy_cfg {
+	unsigned int num_phys;
+	unsigned int phy_ids[MAX_HDPTX_PHY_NUM];
+};
+
 struct rk_hdptx_phy {
 	struct device *dev;
 	struct regmap *regmap;
 	struct regmap *grf;
 
+	/* PHY const config */
+	const struct rk_hdptx_phy_cfg *cfgs;
+	int phy_id;
+
 	struct phy *phy;
 	struct phy_config *phy_cfg;
 	struct clk_bulk_data *clks;
@@ -1857,15 +1868,14 @@ static int rk_hdptx_phy_clk_register(struct rk_hdptx_phy *hdptx)
 	struct device *dev = hdptx->dev;
 	const char *name, *pname;
 	struct clk *refclk;
-	int ret, id;
+	int ret;
 
 	refclk = devm_clk_get(dev, "ref");
 	if (IS_ERR(refclk))
 		return dev_err_probe(dev, PTR_ERR(refclk),
 				     "Failed to get ref clock\n");
 
-	id = of_alias_get_id(dev->of_node, "hdptxphy");
-	name = id > 0 ? "clk_hdmiphy_pixel1" : "clk_hdmiphy_pixel0";
+	name = hdptx->phy_id > 0 ? "clk_hdmiphy_pixel1" : "clk_hdmiphy_pixel0";
 	pname = __clk_get_name(refclk);
 
 	hdptx->hw.init = CLK_HW_INIT(name, pname, &hdptx_phy_clk_ops,
@@ -1908,8 +1918,9 @@ static int rk_hdptx_phy_probe(struct platform_device *pdev)
 	struct phy_provider *phy_provider;
 	struct device *dev = &pdev->dev;
 	struct rk_hdptx_phy *hdptx;
+	struct resource *res;
 	void __iomem *regs;
-	int ret;
+	int ret, id;
 
 	hdptx = devm_kzalloc(dev, sizeof(*hdptx), GFP_KERNEL);
 	if (!hdptx)
@@ -1917,11 +1928,27 @@ static int rk_hdptx_phy_probe(struct platform_device *pdev)
 
 	hdptx->dev = dev;
 
-	regs = devm_platform_ioremap_resource(pdev, 0);
+	regs = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(regs))
 		return dev_err_probe(dev, PTR_ERR(regs),
 				     "Failed to ioremap resource\n");
 
+	hdptx->cfgs = device_get_match_data(dev);
+	if (!hdptx->cfgs)
+		return dev_err_probe(dev, -EINVAL, "missing match data\n");
+
+	/* find the phy-id from the io address */
+	hdptx->phy_id = -ENODEV;
+	for (id = 0; id < hdptx->cfgs->num_phys; id++) {
+		if (res->start == hdptx->cfgs->phy_ids[id]) {
+			hdptx->phy_id = id;
+			break;
+		}
+	}
+
+	if (hdptx->phy_id < 0)
+		return dev_err_probe(dev, -ENODEV, "no matching device found\n");
+
 	ret = devm_clk_bulk_get_all(dev, &hdptx->clks);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "Failed to get clocks\n");
@@ -1981,8 +2008,19 @@ static const struct dev_pm_ops rk_hdptx_phy_pm_ops = {
 		       rk_hdptx_phy_runtime_resume, NULL)
 };
 
+static const struct rk_hdptx_phy_cfg rk3588_hdptx_phy_cfgs = {
+	.num_phys = 2,
+	.phy_ids = {
+		0xfed60000,
+		0xfed70000,
+	},
+};
+
 static const struct of_device_id rk_hdptx_phy_of_match[] = {
-	{ .compatible = "rockchip,rk3588-hdptx-phy", },
+	{
+		.compatible = "rockchip,rk3588-hdptx-phy",
+		.data = &rk3588_hdptx_phy_cfgs
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, rk_hdptx_phy_of_match);
-- 
2.45.2

Re: [PATCH 2/2] phy: phy-rockchip-samsung-hdptx: Don't use dt aliases to determine phy-id

[Cristian Ciocaltea]

On 12/6/24 12:34 PM, Heiko Stuebner wrote:
> From: Heiko Stuebner <heiko.stuebner@cherry.de>
> 
> The phy needs to know its identity in the system (phy0 or phy1 on rk3588)
> for some actions and the driver currently contains code abusing of_alias
> for that.
> 
> Devicetree aliases are always optional and should not be used for core
> device functionality, so instead keep a list of phys on a soc in the
> of_device_data and find the phy-id by comparing against the mapped
> register-base.
> 
> Fixes: c4b09c562086 ("phy: phy-rockchip-samsung-hdptx: Add clock provider support")
> Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> ---
>  .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 50 ++++++++++++++++---
>  1 file changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> index c5c64c209e96..b137f8c4d157 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> @@ -385,11 +385,22 @@ enum rk_hdptx_reset {
>  	RST_MAX
>  };

[...]

> +
> +	/* find the phy-id from the io address */
> +	hdptx->phy_id = -ENODEV;
> +	for (id = 0; id < hdptx->cfgs->num_phys; id++) {
> +		if (res->start == hdptx->cfgs->phy_ids[id]) {
> +			hdptx->phy_id = id;
> +			break;
> +		}
> +	}
> +
> +	if (hdptx->phy_id < 0)
> +		return dev_err_probe(dev, -ENODEV, "no matching device found\n");

Maybe we could simply fallback to assume phy1 doesn't exist in this
case, which avoids the need to provide a match data with a single entry.

Regardless,

Reviewed-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

Re: [PATCH 2/2] phy: phy-rockchip-samsung-hdptx: Don't use dt aliases to determine phy-id

[Heiko Stübner]

Hi Cristian,

Am Freitag, 6. Dezember 2024, 12:26:35 CET schrieb Cristian Ciocaltea:
> On 12/6/24 12:34 PM, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@cherry.de>
> > 
> > The phy needs to know its identity in the system (phy0 or phy1 on rk3588)
> > for some actions and the driver currently contains code abusing of_alias
> > for that.
> > 
> > Devicetree aliases are always optional and should not be used for core
> > device functionality, so instead keep a list of phys on a soc in the
> > of_device_data and find the phy-id by comparing against the mapped
> > register-base.
> > 
> > Fixes: c4b09c562086 ("phy: phy-rockchip-samsung-hdptx: Add clock provider support")
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@cherry.de>
> > ---
> >  .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 50 ++++++++++++++++---
> >  1 file changed, 44 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> > index c5c64c209e96..b137f8c4d157 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> > @@ -385,11 +385,22 @@ enum rk_hdptx_reset {
> >  	RST_MAX
> >  };
> 
> [...]
> 
> > +
> > +	/* find the phy-id from the io address */
> > +	hdptx->phy_id = -ENODEV;
> > +	for (id = 0; id < hdptx->cfgs->num_phys; id++) {
> > +		if (res->start == hdptx->cfgs->phy_ids[id]) {
> > +			hdptx->phy_id = id;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (hdptx->phy_id < 0)
> > +		return dev_err_probe(dev, -ENODEV, "no matching device found\n");
> 
> Maybe we could simply fallback to assume phy1 doesn't exist in this
> case, which avoids the need to provide a match data with a single entry.

Personally I'm a fan of consistent behaviour, not things working
accidentially ;-) . See the usbdp phy for example, also declaring just
one phy using the same mechanism.

Also I really don't trust the hdptxphy-grf being stable over time.
Rockchip engineers always move bts around in those, so there will be a
need for platform-data at some point anyway.


> Regardless,
> 
> Reviewed-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>

thanks :-)

Heiko

Re: [PATCH 0/2] phy: phy-rockchip-samsung-hdptx: don't use of-alias

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 585 bytes --]

Hi,

On Fri, Dec 06, 2024 at 11:33:59AM +0100, Heiko Stuebner wrote:
> One nice comment addition and removal of of_alias usage for core
> device functionality.
> 
> Heiko Stuebner (2):
>   phy: phy-rockchip-samsung-hdptx: annotate regmap register-callback
>   phy: phy-rockchip-samsung-hdptx: Don't use dt aliases to determine
>     phy-id
> 
>  .../phy/rockchip/phy-rockchip-samsung-hdptx.c | 64 +++++++++++++++----
>  1 file changed, 51 insertions(+), 13 deletions(-)

Full series is

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] phy: phy-rockchip-samsung-hdptx: don't use of-alias

[Heiko Stübner]

Hi phy-maintainers,

Am Freitag, 6. Dezember 2024, 11:33:59 MEZ schrieb Heiko Stuebner:
> One nice comment addition and removal of of_alias usage for core
> device functionality.
> 
> Heiko Stuebner (2):
>   phy: phy-rockchip-samsung-hdptx: annotate regmap register-callback
>   phy: phy-rockchip-samsung-hdptx: Don't use dt aliases to determine
>     phy-id

any thoughts on these 2 patches?

Thanks a lot
Heiko

Re: [PATCH 0/2] phy: phy-rockchip-samsung-hdptx: don't use of-alias

[Vinod Koul]

On Fri, 06 Dec 2024 11:33:59 +0100, Heiko Stuebner wrote:
> One nice comment addition and removal of of_alias usage for core
> device functionality.
> 
> Heiko Stuebner (2):
>   phy: phy-rockchip-samsung-hdptx: annotate regmap register-callback
>   phy: phy-rockchip-samsung-hdptx: Don't use dt aliases to determine
>     phy-id
> 
> [...]

Applied, thanks!

[1/2] phy: phy-rockchip-samsung-hdptx: annotate regmap register-callback
      commit: c8f7d65cac565cacf0420acf8b54c855dd7b4484
[2/2] phy: phy-rockchip-samsung-hdptx: Don't use dt aliases to determine phy-id
      commit: f08d1c08563846f9be79a4859e912c8795d690fd

Best regards,
-- 
~Vinod