[PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Frank Wang]

From: Frank Wang <frank.wang@rock-chips.com>

Add compatible for the USB2 phy in the Rockchip RK3576 SoC.

In addition, since the RK3576 introduced the USB MMU, this change
also add the related clock properties for it.

Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
---
Changelog:
v3:
 - narrowed rk3576 clocks by compatible property.

v2:
 - Categorize clock names by oneOf keyword.

v1:
 - https://patchwork.kernel.org/project/linux-phy/patch/20240923025326.10467-1-frank.wang@rock-chips.com/

 .../bindings/phy/rockchip,inno-usb2phy.yaml    | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/phy/rockchip,inno-usb2phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,inno-usb2phy.yaml
index 5254413137c6..d286753bd53f 100644
--- a/Documentation/devicetree/bindings/phy/rockchip,inno-usb2phy.yaml
+++ b/Documentation/devicetree/bindings/phy/rockchip,inno-usb2phy.yaml
@@ -20,6 +20,7 @@ properties:
       - rockchip,rk3366-usb2phy
       - rockchip,rk3399-usb2phy
       - rockchip,rk3568-usb2phy
+      - rockchip,rk3576-usb2phy
       - rockchip,rk3588-usb2phy
       - rockchip,rv1108-usb2phy
 
@@ -143,6 +144,7 @@ allOf:
           contains:
             enum:
               - rockchip,rk3568-usb2phy
+              - rockchip,rk3576-usb2phy
               - rockchip,rk3588-usb2phy
 
     then:
@@ -171,6 +173,22 @@ allOf:
           required:
             - interrupts
             - interrupt-names
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - rockchip,rk3576-usb2phy
+    then:
+      properties:
+        clocks:
+          minItems: 3
+          maxItems: 3
+        clock-names:
+          items:
+            - const: phyclk
+            - const: aclk
+            - const: aclk_slv
 
 additionalProperties: false
 
-- 
2.34.1

[PATCH v3 2/2] phy: rockchip: inno-usb2: Add usb2 phys support for rk3576

[Frank Wang]

From: William Wu <william.wu@rock-chips.com>

The RK3576 SoC has two independent USB2.0 PHYs, and each PHY has
one port.

This change also converts the clock management from single to bulk
as some Rockchip SoCs (e.g RK3576) have more than one clock.

Signed-off-by: William Wu <william.wu@rock-chips.com>
Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
---
Changelog:
v3:
 - amend the commit log adds clocks converting.
 - retrieve the clock by "clks.id" in *_clk480m_register() function.

v2:
 - no changes.
v1:
 - https://patchwork.kernel.org/project/linux-phy/patch/20240923025326.10467-2-frank.wang@rock-chips.com/

 drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 139 +++++++++++++++++-
 1 file changed, 131 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
index 4f71373ae6e1..642c7857c5ae 100644
--- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
@@ -229,9 +229,10 @@ struct rockchip_usb2phy_port {
  * @dev: pointer to device.
  * @grf: General Register Files regmap.
  * @usbgrf: USB General Register Files regmap.
- * @clk: clock struct of phy input clk.
+ * @clks: array of phy input clocks.
  * @clk480m: clock struct of phy output clk.
  * @clk480m_hw: clock struct of phy output clk management.
+ * @num_clks: number of phy input clocks.
  * @phy_reset: phy reset control.
  * @chg_state: states involved in USB charger detection.
  * @chg_type: USB charger types.
@@ -246,9 +247,10 @@ struct rockchip_usb2phy {
 	struct device	*dev;
 	struct regmap	*grf;
 	struct regmap	*usbgrf;
-	struct clk	*clk;
+	struct clk_bulk_data	*clks;
 	struct clk	*clk480m;
 	struct clk_hw	clk480m_hw;
+	int			num_clks;
 	struct reset_control	*phy_reset;
 	enum usb_chg_state	chg_state;
 	enum power_supply_type	chg_type;
@@ -376,7 +378,9 @@ rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
 {
 	struct device_node *node = rphy->dev->of_node;
 	struct clk_init_data init;
+	struct clk *refclk = NULL;
 	const char *clk_name;
+	int i;
 	int ret = 0;
 
 	init.flags = 0;
@@ -386,8 +390,15 @@ rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
 	/* optional override of the clockname */
 	of_property_read_string(node, "clock-output-names", &init.name);
 
-	if (rphy->clk) {
-		clk_name = __clk_get_name(rphy->clk);
+	for (i = 0; i < rphy->num_clks; i++) {
+		if (!strncmp(rphy->clks[i].id, "phyclk", 6)) {
+			refclk = rphy->clks[i].clk;
+			break;
+		}
+	}
+
+	if (!IS_ERR(refclk)) {
+		clk_name = __clk_get_name(refclk);
 		init.parent_names = &clk_name;
 		init.num_parents = 1;
 	} else {
@@ -1406,22 +1417,29 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
 	if (IS_ERR(rphy->phy_reset))
 		return PTR_ERR(rphy->phy_reset);
 
-	rphy->clk = devm_clk_get_optional_enabled(dev, "phyclk");
-	if (IS_ERR(rphy->clk)) {
-		return dev_err_probe(&pdev->dev, PTR_ERR(rphy->clk),
+	ret = devm_clk_bulk_get_all(dev, &rphy->clks);
+	if (ret == -EPROBE_DEFER) {
+		return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
 				     "failed to get phyclk\n");
 	}
 
+	/* Clocks are optional */
+	rphy->num_clks = ret < 0 ? 0 : ret;
+
 	ret = rockchip_usb2phy_clk480m_register(rphy);
 	if (ret) {
 		dev_err(dev, "failed to register 480m output clock\n");
 		return ret;
 	}
 
+	ret = clk_bulk_prepare_enable(rphy->num_clks, rphy->clks);
+	if (ret)
+		return ret;
+
 	if (rphy->phy_cfg->phy_tuning) {
 		ret = rphy->phy_cfg->phy_tuning(rphy);
 		if (ret)
-			return ret;
+			goto disable_clks;
 	}
 
 	index = 0;
@@ -1484,6 +1502,8 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
 
 put_child:
 	of_node_put(child_np);
+disable_clks:
+	clk_bulk_disable_unprepare(rphy->num_clks, rphy->clks);
 	return ret;
 }
 
@@ -1495,6 +1515,30 @@ static int rk3128_usb2phy_tuning(struct rockchip_usb2phy *rphy)
 				BIT(2) << BIT_WRITEABLE_SHIFT | 0);
 }
 
+static int rk3576_usb2phy_tuning(struct rockchip_usb2phy *rphy)
+{
+	int ret;
+	u32 reg = rphy->phy_cfg->reg;
+
+	/* Deassert SIDDQ to power on analog block */
+	ret = regmap_write(rphy->grf, reg + 0x0010, GENMASK(29, 29) | 0x0000);
+	if (ret)
+		return ret;
+
+	/* Do reset after exit IDDQ mode */
+	ret = rockchip_usb2phy_reset(rphy);
+	if (ret)
+		return ret;
+
+	/* HS DC Voltage Level Adjustment 4'b1001 : +5.89% */
+	ret |= regmap_write(rphy->grf, reg + 0x000c, GENMASK(27, 24) | 0x0900);
+
+	/* HS Transmitter Pre-Emphasis Current Control 2'b10 : 2x */
+	ret |= regmap_write(rphy->grf, reg + 0x0010, GENMASK(20, 19) | 0x0010);
+
+	return ret;
+}
+
 static int rk3588_usb2phy_tuning(struct rockchip_usb2phy *rphy)
 {
 	int ret;
@@ -1923,6 +1967,84 @@ static const struct rockchip_usb2phy_cfg rk3568_phy_cfgs[] = {
 	{ /* sentinel */ }
 };
 
+static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
+	{
+		.reg = 0x0,
+		.num_ports	= 1,
+		.phy_tuning	= rk3576_usb2phy_tuning,
+		.clkout_ctl	= { 0x0008, 0, 0, 1, 0 },
+		.port_cfgs	= {
+			[USB2PHY_PORT_OTG] = {
+				.phy_sus	= { 0x0000, 8, 0, 0, 0x1d1 },
+				.bvalid_det_en	= { 0x00c0, 1, 1, 0, 1 },
+				.bvalid_det_st	= { 0x00c4, 1, 1, 0, 1 },
+				.bvalid_det_clr = { 0x00c8, 1, 1, 0, 1 },
+				.ls_det_en	= { 0x00c0, 0, 0, 0, 1 },
+				.ls_det_st	= { 0x00c4, 0, 0, 0, 1 },
+				.ls_det_clr	= { 0x00c8, 0, 0, 0, 1 },
+				.disfall_en	= { 0x00c0, 6, 6, 0, 1 },
+				.disfall_st	= { 0x00c4, 6, 6, 0, 1 },
+				.disfall_clr	= { 0x00c8, 6, 6, 0, 1 },
+				.disrise_en	= { 0x00c0, 5, 5, 0, 1 },
+				.disrise_st	= { 0x00c4, 5, 5, 0, 1 },
+				.disrise_clr	= { 0x00c8, 5, 5, 0, 1 },
+				.utmi_avalid	= { 0x0080, 1, 1, 0, 1 },
+				.utmi_bvalid	= { 0x0080, 0, 0, 0, 1 },
+				.utmi_ls	= { 0x0080, 5, 4, 0, 1 },
+			}
+		},
+		.chg_det = {
+			.cp_det		= { 0x0080, 8, 8, 0, 1 },
+			.dcp_det	= { 0x0080, 8, 8, 0, 1 },
+			.dp_det		= { 0x0080, 9, 9, 1, 0 },
+			.idm_sink_en	= { 0x0010, 5, 5, 1, 0 },
+			.idp_sink_en	= { 0x0010, 5, 5, 0, 1 },
+			.idp_src_en	= { 0x0010, 14, 14, 0, 1 },
+			.rdm_pdwn_en	= { 0x0010, 14, 14, 0, 1 },
+			.vdm_src_en	= { 0x0010, 7, 6, 0, 3 },
+			.vdp_src_en	= { 0x0010, 7, 6, 0, 3 },
+		},
+	},
+	{
+		.reg = 0x2000,
+		.num_ports	= 1,
+		.phy_tuning	= rk3576_usb2phy_tuning,
+		.clkout_ctl	= { 0x2008, 0, 0, 1, 0 },
+		.port_cfgs	= {
+			[USB2PHY_PORT_OTG] = {
+				.phy_sus	= { 0x2000, 8, 0, 0, 0x1d1 },
+				.bvalid_det_en	= { 0x20c0, 1, 1, 0, 1 },
+				.bvalid_det_st	= { 0x20c4, 1, 1, 0, 1 },
+				.bvalid_det_clr = { 0x20c8, 1, 1, 0, 1 },
+				.ls_det_en	= { 0x20c0, 0, 0, 0, 1 },
+				.ls_det_st	= { 0x20c4, 0, 0, 0, 1 },
+				.ls_det_clr	= { 0x20c8, 0, 0, 0, 1 },
+				.disfall_en	= { 0x20c0, 6, 6, 0, 1 },
+				.disfall_st	= { 0x20c4, 6, 6, 0, 1 },
+				.disfall_clr	= { 0x20c8, 6, 6, 0, 1 },
+				.disrise_en	= { 0x20c0, 5, 5, 0, 1 },
+				.disrise_st	= { 0x20c4, 5, 5, 0, 1 },
+				.disrise_clr	= { 0x20c8, 5, 5, 0, 1 },
+				.utmi_avalid	= { 0x2080, 1, 1, 0, 1 },
+				.utmi_bvalid	= { 0x2080, 0, 0, 0, 1 },
+				.utmi_ls	= { 0x2080, 5, 4, 0, 1 },
+			}
+		},
+		.chg_det = {
+			.cp_det		= { 0x2080, 8, 8, 0, 1 },
+			.dcp_det	= { 0x2080, 8, 8, 0, 1 },
+			.dp_det		= { 0x2080, 9, 9, 1, 0 },
+			.idm_sink_en	= { 0x2010, 5, 5, 1, 0 },
+			.idp_sink_en	= { 0x2010, 5, 5, 0, 1 },
+			.idp_src_en	= { 0x2010, 14, 14, 0, 1 },
+			.rdm_pdwn_en	= { 0x2010, 14, 14, 0, 1 },
+			.vdm_src_en	= { 0x2010, 7, 6, 0, 3 },
+			.vdp_src_en	= { 0x2010, 7, 6, 0, 3 },
+		},
+	},
+	{ /* sentinel */ }
+};
+
 static const struct rockchip_usb2phy_cfg rk3588_phy_cfgs[] = {
 	{
 		.reg = 0x0000,
@@ -2094,6 +2216,7 @@ static const struct of_device_id rockchip_usb2phy_dt_match[] = {
 	{ .compatible = "rockchip,rk3366-usb2phy", .data = &rk3366_phy_cfgs },
 	{ .compatible = "rockchip,rk3399-usb2phy", .data = &rk3399_phy_cfgs },
 	{ .compatible = "rockchip,rk3568-usb2phy", .data = &rk3568_phy_cfgs },
+	{ .compatible = "rockchip,rk3576-usb2phy", .data = &rk3576_phy_cfgs },
 	{ .compatible = "rockchip,rk3588-usb2phy", .data = &rk3588_phy_cfgs },
 	{ .compatible = "rockchip,rv1108-usb2phy", .data = &rv1108_phy_cfgs },
 	{}
-- 
2.34.1

Re: [PATCH v3 2/2] phy: rockchip: inno-usb2: Add usb2 phys support for rk3576

[Heiko Stuebner]

Am Donnerstag, 26. September 2024, 12:32:23 CEST schrieb Frank Wang:
> From: William Wu <william.wu@rock-chips.com>
> 
> The RK3576 SoC has two independent USB2.0 PHYs, and each PHY has
> one port.
> 
> This change also converts the clock management from single to bulk
> as some Rockchip SoCs (e.g RK3576) have more than one clock.
> 
> Signed-off-by: William Wu <william.wu@rock-chips.com>
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> ---
> Changelog:
> v3:
>  - amend the commit log adds clocks converting.
>  - retrieve the clock by "clks.id" in *_clk480m_register() function.
> 
> v2:
>  - no changes.
> v1:
>  - https://patchwork.kernel.org/project/linux-phy/patch/20240923025326.10467-2-frank.wang@rock-chips.com/
> 
>  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 139 +++++++++++++++++-
>  1 file changed, 131 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> index 4f71373ae6e1..642c7857c5ae 100644
> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> @@ -229,9 +229,10 @@ struct rockchip_usb2phy_port {
>   * @dev: pointer to device.
>   * @grf: General Register Files regmap.
>   * @usbgrf: USB General Register Files regmap.
> - * @clk: clock struct of phy input clk.
> + * @clks: array of phy input clocks.
>   * @clk480m: clock struct of phy output clk.
>   * @clk480m_hw: clock struct of phy output clk management.
> + * @num_clks: number of phy input clocks.
>   * @phy_reset: phy reset control.
>   * @chg_state: states involved in USB charger detection.
>   * @chg_type: USB charger types.
> @@ -246,9 +247,10 @@ struct rockchip_usb2phy {
>  	struct device	*dev;
>  	struct regmap	*grf;
>  	struct regmap	*usbgrf;
> -	struct clk	*clk;
> +	struct clk_bulk_data	*clks;
>  	struct clk	*clk480m;
>  	struct clk_hw	clk480m_hw;
> +	int			num_clks;
>  	struct reset_control	*phy_reset;
>  	enum usb_chg_state	chg_state;
>  	enum power_supply_type	chg_type;
> @@ -376,7 +378,9 @@ rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
>  {
>  	struct device_node *node = rphy->dev->of_node;
>  	struct clk_init_data init;
> +	struct clk *refclk = NULL;
>  	const char *clk_name;
> +	int i;
>  	int ret = 0;
>  
>  	init.flags = 0;
> @@ -386,8 +390,15 @@ rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
>  	/* optional override of the clockname */
>  	of_property_read_string(node, "clock-output-names", &init.name);
>  
> -	if (rphy->clk) {
> -		clk_name = __clk_get_name(rphy->clk);
> +	for (i = 0; i < rphy->num_clks; i++) {
> +		if (!strncmp(rphy->clks[i].id, "phyclk", 6)) {
> +			refclk = rphy->clks[i].clk;
> +			break;
> +		}
> +	}
> +
> +	if (!IS_ERR(refclk)) {
> +		clk_name = __clk_get_name(refclk);
>  		init.parent_names = &clk_name;
>  		init.num_parents = 1;
>  	} else {
> @@ -1406,22 +1417,29 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
>  	if (IS_ERR(rphy->phy_reset))
>  		return PTR_ERR(rphy->phy_reset);
>  
> -	rphy->clk = devm_clk_get_optional_enabled(dev, "phyclk");
> -	if (IS_ERR(rphy->clk)) {
> -		return dev_err_probe(&pdev->dev, PTR_ERR(rphy->clk),
> +	ret = devm_clk_bulk_get_all(dev, &rphy->clks);
> +	if (ret == -EPROBE_DEFER) {
> +		return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
>  				     "failed to get phyclk\n");
>  	}
>  
> +	/* Clocks are optional */
> +	rphy->num_clks = ret < 0 ? 0 : ret;
> +
>  	ret = rockchip_usb2phy_clk480m_register(rphy);
>  	if (ret) {
>  		dev_err(dev, "failed to register 480m output clock\n");
>  		return ret;
>  	}
>  
> +	ret = clk_bulk_prepare_enable(rphy->num_clks, rphy->clks);
> +	if (ret)
> +		return ret;
> +

Do you actually need that separate enable step?
There exists devm_clk_bulk_get_all_enable() 

https://elixir.bootlin.com/linux/v6.11/source/include/linux/clk.h#L511

which you could use above. Especially as otherwise you'd need a remove
callback to disable the clock on driver unbind?


>  	if (rphy->phy_cfg->phy_tuning) {
>  		ret = rphy->phy_cfg->phy_tuning(rphy);
>  		if (ret)
> -			return ret;
> +			goto disable_clks;
>  	}
>  
>  	index = 0;
> @@ -1484,6 +1502,8 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
>  
>  put_child:
>  	of_node_put(child_np);
> +disable_clks:
> +	clk_bulk_disable_unprepare(rphy->num_clks, rphy->clks);
>  	return ret;
>  }
>  
> @@ -1495,6 +1515,30 @@ static int rk3128_usb2phy_tuning(struct rockchip_usb2phy *rphy)
>  				BIT(2) << BIT_WRITEABLE_SHIFT | 0);
>  }


I still maintain, that this maybe should be two separate patches.
And you even seem to have nice "cut-here" in it ;-)

The code above converts the driver to use clk_bulk to handle
multiple clocks for controller variants needing it.


And the code below adds the rk3576-specific data to the driver.



> +static int rk3576_usb2phy_tuning(struct rockchip_usb2phy *rphy)
> +{
> +	int ret;
> +	u32 reg = rphy->phy_cfg->reg;
> +
> +	/* Deassert SIDDQ to power on analog block */
> +	ret = regmap_write(rphy->grf, reg + 0x0010, GENMASK(29, 29) | 0x0000);
> +	if (ret)
> +		return ret;
> +
> +	/* Do reset after exit IDDQ mode */
> +	ret = rockchip_usb2phy_reset(rphy);
> +	if (ret)
> +		return ret;
> +
> +	/* HS DC Voltage Level Adjustment 4'b1001 : +5.89% */
> +	ret |= regmap_write(rphy->grf, reg + 0x000c, GENMASK(27, 24) | 0x0900);
> +
> +	/* HS Transmitter Pre-Emphasis Current Control 2'b10 : 2x */
> +	ret |= regmap_write(rphy->grf, reg + 0x0010, GENMASK(20, 19) | 0x0010);
> +
> +	return ret;
> +}
> +
>  static int rk3588_usb2phy_tuning(struct rockchip_usb2phy *rphy)
>  {
>  	int ret;
> @@ -1923,6 +1967,84 @@ static const struct rockchip_usb2phy_cfg rk3568_phy_cfgs[] = {
>  	{ /* sentinel */ }
>  };
>  
> +static const struct rockchip_usb2phy_cfg rk3576_phy_cfgs[] = {
> +	{
> +		.reg = 0x0,
> +		.num_ports	= 1,
> +		.phy_tuning	= rk3576_usb2phy_tuning,
> +		.clkout_ctl	= { 0x0008, 0, 0, 1, 0 },
> +		.port_cfgs	= {
> +			[USB2PHY_PORT_OTG] = {
> +				.phy_sus	= { 0x0000, 8, 0, 0, 0x1d1 },
> +				.bvalid_det_en	= { 0x00c0, 1, 1, 0, 1 },
> +				.bvalid_det_st	= { 0x00c4, 1, 1, 0, 1 },
> +				.bvalid_det_clr = { 0x00c8, 1, 1, 0, 1 },
> +				.ls_det_en	= { 0x00c0, 0, 0, 0, 1 },
> +				.ls_det_st	= { 0x00c4, 0, 0, 0, 1 },
> +				.ls_det_clr	= { 0x00c8, 0, 0, 0, 1 },
> +				.disfall_en	= { 0x00c0, 6, 6, 0, 1 },
> +				.disfall_st	= { 0x00c4, 6, 6, 0, 1 },
> +				.disfall_clr	= { 0x00c8, 6, 6, 0, 1 },
> +				.disrise_en	= { 0x00c0, 5, 5, 0, 1 },
> +				.disrise_st	= { 0x00c4, 5, 5, 0, 1 },
> +				.disrise_clr	= { 0x00c8, 5, 5, 0, 1 },
> +				.utmi_avalid	= { 0x0080, 1, 1, 0, 1 },
> +				.utmi_bvalid	= { 0x0080, 0, 0, 0, 1 },
> +				.utmi_ls	= { 0x0080, 5, 4, 0, 1 },
> +			}
> +		},
> +		.chg_det = {
> +			.cp_det		= { 0x0080, 8, 8, 0, 1 },
> +			.dcp_det	= { 0x0080, 8, 8, 0, 1 },
> +			.dp_det		= { 0x0080, 9, 9, 1, 0 },
> +			.idm_sink_en	= { 0x0010, 5, 5, 1, 0 },
> +			.idp_sink_en	= { 0x0010, 5, 5, 0, 1 },
> +			.idp_src_en	= { 0x0010, 14, 14, 0, 1 },
> +			.rdm_pdwn_en	= { 0x0010, 14, 14, 0, 1 },
> +			.vdm_src_en	= { 0x0010, 7, 6, 0, 3 },
> +			.vdp_src_en	= { 0x0010, 7, 6, 0, 3 },
> +		},
> +	},
> +	{
> +		.reg = 0x2000,
> +		.num_ports	= 1,
> +		.phy_tuning	= rk3576_usb2phy_tuning,
> +		.clkout_ctl	= { 0x2008, 0, 0, 1, 0 },
> +		.port_cfgs	= {
> +			[USB2PHY_PORT_OTG] = {
> +				.phy_sus	= { 0x2000, 8, 0, 0, 0x1d1 },
> +				.bvalid_det_en	= { 0x20c0, 1, 1, 0, 1 },
> +				.bvalid_det_st	= { 0x20c4, 1, 1, 0, 1 },
> +				.bvalid_det_clr = { 0x20c8, 1, 1, 0, 1 },
> +				.ls_det_en	= { 0x20c0, 0, 0, 0, 1 },
> +				.ls_det_st	= { 0x20c4, 0, 0, 0, 1 },
> +				.ls_det_clr	= { 0x20c8, 0, 0, 0, 1 },
> +				.disfall_en	= { 0x20c0, 6, 6, 0, 1 },
> +				.disfall_st	= { 0x20c4, 6, 6, 0, 1 },
> +				.disfall_clr	= { 0x20c8, 6, 6, 0, 1 },
> +				.disrise_en	= { 0x20c0, 5, 5, 0, 1 },
> +				.disrise_st	= { 0x20c4, 5, 5, 0, 1 },
> +				.disrise_clr	= { 0x20c8, 5, 5, 0, 1 },
> +				.utmi_avalid	= { 0x2080, 1, 1, 0, 1 },
> +				.utmi_bvalid	= { 0x2080, 0, 0, 0, 1 },
> +				.utmi_ls	= { 0x2080, 5, 4, 0, 1 },
> +			}
> +		},
> +		.chg_det = {
> +			.cp_det		= { 0x2080, 8, 8, 0, 1 },
> +			.dcp_det	= { 0x2080, 8, 8, 0, 1 },
> +			.dp_det		= { 0x2080, 9, 9, 1, 0 },
> +			.idm_sink_en	= { 0x2010, 5, 5, 1, 0 },
> +			.idp_sink_en	= { 0x2010, 5, 5, 0, 1 },
> +			.idp_src_en	= { 0x2010, 14, 14, 0, 1 },
> +			.rdm_pdwn_en	= { 0x2010, 14, 14, 0, 1 },
> +			.vdm_src_en	= { 0x2010, 7, 6, 0, 3 },
> +			.vdp_src_en	= { 0x2010, 7, 6, 0, 3 },
> +		},
> +	},
> +	{ /* sentinel */ }
> +};
> +
>  static const struct rockchip_usb2phy_cfg rk3588_phy_cfgs[] = {
>  	{
>  		.reg = 0x0000,
> @@ -2094,6 +2216,7 @@ static const struct of_device_id rockchip_usb2phy_dt_match[] = {
>  	{ .compatible = "rockchip,rk3366-usb2phy", .data = &rk3366_phy_cfgs },
>  	{ .compatible = "rockchip,rk3399-usb2phy", .data = &rk3399_phy_cfgs },
>  	{ .compatible = "rockchip,rk3568-usb2phy", .data = &rk3568_phy_cfgs },
> +	{ .compatible = "rockchip,rk3576-usb2phy", .data = &rk3576_phy_cfgs },
>  	{ .compatible = "rockchip,rk3588-usb2phy", .data = &rk3588_phy_cfgs },
>  	{ .compatible = "rockchip,rv1108-usb2phy", .data = &rv1108_phy_cfgs },
>  	{}
> 

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Krzysztof Kozlowski]

On 26/09/2024 12:32, Frank Wang wrote:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - rockchip,rk3576-usb2phy
> +    then:
> +      properties:
> +        clocks:
> +          minItems: 3
> +          maxItems: 3

Read one more time the example I gave you. Top-level constraints are
saying max one clock.

Best regards,
Krzysztof

Re: [PATCH v3 2/2] phy: rockchip: inno-usb2: Add usb2 phys support for rk3576

[Frank Wang]

Hi Heiko,

On 2024/9/26 18:48, Heiko Stuebner wrote:
> Am Donnerstag, 26. September 2024, 12:32:23 CEST schrieb Frank Wang:
>> From: William Wu <william.wu@rock-chips.com>
>>
>> The RK3576 SoC has two independent USB2.0 PHYs, and each PHY has
>> one port.
>>
>> This change also converts the clock management from single to bulk
>> as some Rockchip SoCs (e.g RK3576) have more than one clock.
>>
>> Signed-off-by: William Wu <william.wu@rock-chips.com>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> ---
>> Changelog:
>> v3:
>>   - amend the commit log adds clocks converting.
>>   - retrieve the clock by "clks.id" in *_clk480m_register() function.
>>
>> v2:
>>   - no changes.
>> v1:
>>   - https://patchwork.kernel.org/project/linux-phy/patch/20240923025326.10467-2-frank.wang@rock-chips.com/
>>
>>   drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 139 +++++++++++++++++-
>>   1 file changed, 131 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>> index 4f71373ae6e1..642c7857c5ae 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>> @@ -229,9 +229,10 @@ struct rockchip_usb2phy_port {
>>    * @dev: pointer to device.
>>    * @grf: General Register Files regmap.
>>    * @usbgrf: USB General Register Files regmap.
>> - * @clk: clock struct of phy input clk.
>> + * @clks: array of phy input clocks.
>>    * @clk480m: clock struct of phy output clk.
>>    * @clk480m_hw: clock struct of phy output clk management.
>> + * @num_clks: number of phy input clocks.
>>    * @phy_reset: phy reset control.
>>    * @chg_state: states involved in USB charger detection.
>>    * @chg_type: USB charger types.
>> @@ -246,9 +247,10 @@ struct rockchip_usb2phy {
>>   	struct device	*dev;
>>   	struct regmap	*grf;
>>   	struct regmap	*usbgrf;
>> -	struct clk	*clk;
>> +	struct clk_bulk_data	*clks;
>>   	struct clk	*clk480m;
>>   	struct clk_hw	clk480m_hw;
>> +	int			num_clks;
>>   	struct reset_control	*phy_reset;
>>   	enum usb_chg_state	chg_state;
>>   	enum power_supply_type	chg_type;
>> @@ -376,7 +378,9 @@ rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
>>   {
>>   	struct device_node *node = rphy->dev->of_node;
>>   	struct clk_init_data init;
>> +	struct clk *refclk = NULL;
>>   	const char *clk_name;
>> +	int i;
>>   	int ret = 0;
>>   
>>   	init.flags = 0;
>> @@ -386,8 +390,15 @@ rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
>>   	/* optional override of the clockname */
>>   	of_property_read_string(node, "clock-output-names", &init.name);
>>   
>> -	if (rphy->clk) {
>> -		clk_name = __clk_get_name(rphy->clk);
>> +	for (i = 0; i < rphy->num_clks; i++) {
>> +		if (!strncmp(rphy->clks[i].id, "phyclk", 6)) {
>> +			refclk = rphy->clks[i].clk;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!IS_ERR(refclk)) {
>> +		clk_name = __clk_get_name(refclk);
>>   		init.parent_names = &clk_name;
>>   		init.num_parents = 1;
>>   	} else {
>> @@ -1406,22 +1417,29 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
>>   	if (IS_ERR(rphy->phy_reset))
>>   		return PTR_ERR(rphy->phy_reset);
>>   
>> -	rphy->clk = devm_clk_get_optional_enabled(dev, "phyclk");
>> -	if (IS_ERR(rphy->clk)) {
>> -		return dev_err_probe(&pdev->dev, PTR_ERR(rphy->clk),
>> +	ret = devm_clk_bulk_get_all(dev, &rphy->clks);
>> +	if (ret == -EPROBE_DEFER) {
>> +		return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
>>   				     "failed to get phyclk\n");
>>   	}
>>   
>> +	/* Clocks are optional */
>> +	rphy->num_clks = ret < 0 ? 0 : ret;
>> +
>>   	ret = rockchip_usb2phy_clk480m_register(rphy);
>>   	if (ret) {
>>   		dev_err(dev, "failed to register 480m output clock\n");
>>   		return ret;
>>   	}
>>   
>> +	ret = clk_bulk_prepare_enable(rphy->num_clks, rphy->clks);
>> +	if (ret)
>> +		return ret;
>> +
> Do you actually need that separate enable step?
> There exists devm_clk_bulk_get_all_enable()
>
> https://elixir.bootlin.com/linux/v6.11/source/include/linux/clk.h#L511
>
> which you could use above. Especially as otherwise you'd need a remove
> callback to disable the clock on driver unbind?
>

Using devm_clk_bulk_get_all() just can get clocknumbers, 
anddevm_clk_bulk_get_all_enable() is not.
It seems that there is no other API can get the number of clocks separately.


>>   	if (rphy->phy_cfg->phy_tuning) {
>>   		ret = rphy->phy_cfg->phy_tuning(rphy);
>>   		if (ret)
>> -			return ret;
>> +			goto disable_clks;
>>   	}
>>   
>>   	index = 0;
>> @@ -1484,6 +1502,8 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
>>   
>>   put_child:
>>   	of_node_put(child_np);
>> +disable_clks:
>> +	clk_bulk_disable_unprepare(rphy->num_clks, rphy->clks);
>>   	return ret;
>>   }
>>   
>> @@ -1495,6 +1515,30 @@ static int rk3128_usb2phy_tuning(struct rockchip_usb2phy *rphy)
>>   				BIT(2) << BIT_WRITEABLE_SHIFT | 0);
>>   }
>
> I still maintain, that this maybe should be two separate patches.
> And you even seem to have nice "cut-here" in it ;-)
>
> The code above converts the driver to use clk_bulk to handle
> multiple clocks for controller variants needing it.
>
>
> And the code below adds the rk3576-specific data to the driver.
>

Make sense, I shall split two separate patches in the next version.
BTW, should I send the clk_bulk process patch as a separate series? I 
mean I shouldn't send it together with this series, right?


Best regards,
Frank

>
>> [...]
>>
>> +
>>   static const struct rockchip_usb2phy_cfg rk3588_phy_cfgs[] = {
>>   	{
>>   		.reg = 0x0000,
>> @@ -2094,6 +2216,7 @@ static const struct of_device_id rockchip_usb2phy_dt_match[] = {
>>   	{ .compatible = "rockchip,rk3366-usb2phy", .data = &rk3366_phy_cfgs },
>>   	{ .compatible = "rockchip,rk3399-usb2phy", .data = &rk3399_phy_cfgs },
>>   	{ .compatible = "rockchip,rk3568-usb2phy", .data = &rk3568_phy_cfgs },
>> +	{ .compatible = "rockchip,rk3576-usb2phy", .data = &rk3576_phy_cfgs },
>>   	{ .compatible = "rockchip,rk3588-usb2phy", .data = &rk3588_phy_cfgs },
>>   	{ .compatible = "rockchip,rv1108-usb2phy", .data = &rv1108_phy_cfgs },
>>   	{}
>>
>

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Frank Wang]

Hi Krzysztof,

On 2024/9/26 22:19, Krzysztof Kozlowski wrote:
> On 26/09/2024 12:32, Frank Wang wrote:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - rockchip,rk3576-usb2phy
>> +    then:
>> +      properties:
>> +        clocks:
>> +          minItems: 3
>> +          maxItems: 3
> Read one more time the example I gave you. Top-level constraints are
> saying max one clock.
>
> Best regards,
> Krzysztof
>

Sorry for overlooking this, I will set both "clocks" and "clock-names" 
to true, and add the else case below the above codes for the "old" SoCs.
Just like the below.

-  clocks:
-    maxItems: 1
+  clocks: true

-  clock-names:
-    const: phyclk
+  clock-names: true

    assigned-clocks:
      description:
@@ -189,6 +187,13 @@ allOf:
              - const: phyclk
              - const: aclk
              - const: aclk_slv
+    else:
+      properties:
+        clocks:
+          maxItems: 1
+        clock-names:
+          const: phyclk
+


Best regards,
Frank

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Krzysztof Kozlowski]

On 27/09/2024 09:01, Frank Wang wrote:
> Hi Krzysztof,
> 
> On 2024/9/26 22:19, Krzysztof Kozlowski wrote:
>> On 26/09/2024 12:32, Frank Wang wrote:
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - rockchip,rk3576-usb2phy
>>> +    then:
>>> +      properties:
>>> +        clocks:
>>> +          minItems: 3
>>> +          maxItems: 3
>> Read one more time the example I gave you. Top-level constraints are
>> saying max one clock.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Sorry for overlooking this, I will set both "clocks" and "clock-names" 
> to true, and add the else case below the above codes for the "old" SoCs.
> Just like the below.
> 
> -  clocks:
> -    maxItems: 1
> +  clocks: true
> 
> -  clock-names:
> -    const: phyclk
> +  clock-names: true

For the third time, read the code I gave you. Do you see something like
this there? Why doing all the time something different than existing code?

Best regards,
Krzysztof

Re: [PATCH v3 2/2] phy: rockchip: inno-usb2: Add usb2 phys support for rk3576

[Heiko Stuebner]

Hi Frank,

Am Freitag, 27. September 2024, 08:16:42 CEST schrieb Frank Wang:
> On 2024/9/26 18:48, Heiko Stuebner wrote:
> > Am Donnerstag, 26. September 2024, 12:32:23 CEST schrieb Frank Wang:
> >> From: William Wu <william.wu@rock-chips.com>
> >>
> >> The RK3576 SoC has two independent USB2.0 PHYs, and each PHY has
> >> one port.
> >>
> >> This change also converts the clock management from single to bulk
> >> as some Rockchip SoCs (e.g RK3576) have more than one clock.
> >>
> >> Signed-off-by: William Wu <william.wu@rock-chips.com>
> >> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> >> ---
> >> Changelog:
> >> v3:
> >>   - amend the commit log adds clocks converting.
> >>   - retrieve the clock by "clks.id" in *_clk480m_register() function.
> >>
> >> v2:
> >>   - no changes.
> >> v1:
> >>   - https://patchwork.kernel.org/project/linux-phy/patch/20240923025326.10467-2-frank.wang@rock-chips.com/
> >>
> >>   drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 139 +++++++++++++++++-
> >>   1 file changed, 131 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> >> index 4f71373ae6e1..642c7857c5ae 100644
> >> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> >> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> >> @@ -229,9 +229,10 @@ struct rockchip_usb2phy_port {
> >>    * @dev: pointer to device.
> >>    * @grf: General Register Files regmap.
> >>    * @usbgrf: USB General Register Files regmap.
> >> - * @clk: clock struct of phy input clk.
> >> + * @clks: array of phy input clocks.
> >>    * @clk480m: clock struct of phy output clk.
> >>    * @clk480m_hw: clock struct of phy output clk management.
> >> + * @num_clks: number of phy input clocks.
> >>    * @phy_reset: phy reset control.
> >>    * @chg_state: states involved in USB charger detection.
> >>    * @chg_type: USB charger types.
> >> @@ -246,9 +247,10 @@ struct rockchip_usb2phy {
> >>   	struct device	*dev;
> >>   	struct regmap	*grf;
> >>   	struct regmap	*usbgrf;
> >> -	struct clk	*clk;
> >> +	struct clk_bulk_data	*clks;
> >>   	struct clk	*clk480m;
> >>   	struct clk_hw	clk480m_hw;
> >> +	int			num_clks;
> >>   	struct reset_control	*phy_reset;
> >>   	enum usb_chg_state	chg_state;
> >>   	enum power_supply_type	chg_type;
> >> @@ -376,7 +378,9 @@ rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
> >>   {
> >>   	struct device_node *node = rphy->dev->of_node;
> >>   	struct clk_init_data init;
> >> +	struct clk *refclk = NULL;
> >>   	const char *clk_name;
> >> +	int i;
> >>   	int ret = 0;
> >>   
> >>   	init.flags = 0;
> >> @@ -386,8 +390,15 @@ rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
> >>   	/* optional override of the clockname */
> >>   	of_property_read_string(node, "clock-output-names", &init.name);
> >>   
> >> -	if (rphy->clk) {
> >> -		clk_name = __clk_get_name(rphy->clk);
> >> +	for (i = 0; i < rphy->num_clks; i++) {
> >> +		if (!strncmp(rphy->clks[i].id, "phyclk", 6)) {
> >> +			refclk = rphy->clks[i].clk;
> >> +			break;
> >> +		}
> >> +	}
> >> +
> >> +	if (!IS_ERR(refclk)) {
> >> +		clk_name = __clk_get_name(refclk);
> >>   		init.parent_names = &clk_name;
> >>   		init.num_parents = 1;
> >>   	} else {
> >> @@ -1406,22 +1417,29 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
> >>   	if (IS_ERR(rphy->phy_reset))
> >>   		return PTR_ERR(rphy->phy_reset);
> >>   
> >> -	rphy->clk = devm_clk_get_optional_enabled(dev, "phyclk");
> >> -	if (IS_ERR(rphy->clk)) {
> >> -		return dev_err_probe(&pdev->dev, PTR_ERR(rphy->clk),
> >> +	ret = devm_clk_bulk_get_all(dev, &rphy->clks);
> >> +	if (ret == -EPROBE_DEFER) {
> >> +		return dev_err_probe(&pdev->dev, -EPROBE_DEFER,
> >>   				     "failed to get phyclk\n");
> >>   	}
> >>   
> >> +	/* Clocks are optional */
> >> +	rphy->num_clks = ret < 0 ? 0 : ret;
> >> +
> >>   	ret = rockchip_usb2phy_clk480m_register(rphy);
> >>   	if (ret) {
> >>   		dev_err(dev, "failed to register 480m output clock\n");
> >>   		return ret;
> >>   	}
> >>   
> >> +	ret = clk_bulk_prepare_enable(rphy->num_clks, rphy->clks);
> >> +	if (ret)
> >> +		return ret;
> >> +
> > Do you actually need that separate enable step?
> > There exists devm_clk_bulk_get_all_enable()
> >
> > https://elixir.bootlin.com/linux/v6.11/source/include/linux/clk.h#L511
> >
> > which you could use above. Especially as otherwise you'd need a remove
> > callback to disable the clock on driver unbind?
> >
> 
> Using devm_clk_bulk_get_all() just can get clocknumbers, 
> and devm_clk_bulk_get_all_enable() is not.
> It seems that there is no other API can get the number of clocks separately.

Hmm, didn't see that yesterday, but you're right.

From a consistency standpoint I think it would actually make sense to
return the number of clocks from devm_clk_bulk_get_all_enable() .

Because right now, those clk_* functions are looking pretty similar
but behave differently. And looking at the function, the enable-variant
also just uses clk_bulk_get_all, just looses the number of clocks
because "ret" is reused by the enable call.


But if you don't want to touch the clk API that's also ok, but
you should then add a devm_add_action_or_reset call, to disable
the clocks on driver removal, before they are freed.


> >>   	if (rphy->phy_cfg->phy_tuning) {
> >>   		ret = rphy->phy_cfg->phy_tuning(rphy);
> >>   		if (ret)
> >> -			return ret;
> >> +			goto disable_clks;
> >>   	}
> >>   
> >>   	index = 0;
> >> @@ -1484,6 +1502,8 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
> >>   
> >>   put_child:
> >>   	of_node_put(child_np);
> >> +disable_clks:
> >> +	clk_bulk_disable_unprepare(rphy->num_clks, rphy->clks);
> >>   	return ret;
> >>   }
> >>   
> >> @@ -1495,6 +1515,30 @@ static int rk3128_usb2phy_tuning(struct rockchip_usb2phy *rphy)
> >>   				BIT(2) << BIT_WRITEABLE_SHIFT | 0);
> >>   }
> >
> > I still maintain, that this maybe should be two separate patches.
> > And you even seem to have nice "cut-here" in it ;-)
> >
> > The code above converts the driver to use clk_bulk to handle
> > multiple clocks for controller variants needing it.
> >
> >
> > And the code below adds the rk3576-specific data to the driver.
> >
> 
> Make sense, I shall split two separate patches in the next version.
> BTW, should I send the clk_bulk process patch as a separate series? I 
> mean I shouldn't send it together with this series, right?

No, just one series, convert the driver to clk_bulk*, add rk3576 binding,
add rk3576-specific data :-) .

Because people will want to see where you're going with your change
and having "this series depends on this separate patch" creates confusion
and you're really only touching this one driver.

Heiko

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Frank Wang]

Hi Krzysztof,

On 2024/9/27 15:30, Krzysztof Kozlowski wrote:
> On 27/09/2024 09:01, Frank Wang wrote:
>> Hi Krzysztof,
>>
>> On 2024/9/26 22:19, Krzysztof Kozlowski wrote:
>>> On 26/09/2024 12:32, Frank Wang wrote:
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - rockchip,rk3576-usb2phy
>>>> +    then:
>>>> +      properties:
>>>> +        clocks:
>>>> +          minItems: 3
>>>> +          maxItems: 3
>>> Read one more time the example I gave you. Top-level constraints are
>>> saying max one clock.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> Sorry for overlooking this, I will set both "clocks" and "clock-names"
>> to true, and add the else case below the above codes for the "old" SoCs.
>> Just like the below.
>>
>> -  clocks:
>> -    maxItems: 1
>> +  clocks: true
>>
>> -  clock-names:
>> -    const: phyclk
>> +  clock-names: true
> For the third time, read the code I gave you. Do you see something like
> this there? Why doing all the time something different than existing code?

Refer to the link you sent me that I must add minItems property for 
clocks, just like the below codes:

@@ -35,7 +35,8 @@ properties:
      const: 0

    clocks:
-    maxItems: 1
+    minItems: 1
+    maxItems: 3

That can pass dt_binding and dtb checking, however, "clocks" is the 
optional property for some old Rockchip PHYs,  I am not sure is it right 
to force set  minItems as 1 .
If just keep maxItems, the dt_binding checking is failure.


Best regards,
Frank

> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Heiko Stuebner]

Hi Krzysztof,

Am Freitag, 27. September 2024, 09:30:30 CEST schrieb Krzysztof Kozlowski:
> On 27/09/2024 09:01, Frank Wang wrote:
> > Hi Krzysztof,
> > 
> > On 2024/9/26 22:19, Krzysztof Kozlowski wrote:
> >> On 26/09/2024 12:32, Frank Wang wrote:
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            enum:
> >>> +              - rockchip,rk3576-usb2phy
> >>> +    then:
> >>> +      properties:
> >>> +        clocks:
> >>> +          minItems: 3
> >>> +          maxItems: 3
> >> Read one more time the example I gave you. Top-level constraints are
> >> saying max one clock.
> >>
> >> Best regards,
> >> Krzysztof
> >>
> > 
> > Sorry for overlooking this, I will set both "clocks" and "clock-names" 
> > to true, and add the else case below the above codes for the "old" SoCs.
> > Just like the below.
> > 
> > -  clocks:
> > -    maxItems: 1
> > +  clocks: true
> > 
> > -  clock-names:
> > -    const: phyclk
> > +  clock-names: true
> 
> For the third time, read the code I gave you. Do you see something like
> this there? Why doing all the time something different than existing code?

On vacation right now so late to the party, and somewhat confused :-) .

I've tried to find the code you mentioned, but did fail.
In [0] you mention "maybe oneOf". The other replies in that version were
about the ordering needing to stay for the older phy variants.

[1] in v2 has that NAK thing and [2] from v3 references that example again

I am probably just blind, but could use a pointer.


Because I think going with a
  - if:
      properties:
        compatible:
          contains:
            enum:
              - rockchip,rk3576-usb2phy
    then:
      properties:
        clocks:
          minItems: 3
          maxItems: 3
        clock-names:
          items:
            - const: phyclk
            - const: aclk
            - const: aclk_slv
    else:
      properties:
        clocks:
          maxItems: 1
        clock-names:
          const: phyclk

block should actually make sure each variant will check for the appropriate
number of clocks?

And having clocks:true in the main part then makes sure that the property
is not getting marked as:
arch/arm64/boot/dts/rockchip/rk3576-armsom-sige5.dtb: usb2-phy@0: 'clock-names', 'clocks' do not match any of the regexes: 'pinctrl-[0-9]+'
        from schema $id: http://devicetree.org/schemas/phy/rockchip,inno-usb2phy.yaml#


Heiko


[0] https://lore.kernel.org/lkml/snccizbw6thn3lhwad4xppp7vqii4p56ttl2gufwc3ke7vfckf@e4b7nvwwtdfr/
[1] https://lore.kernel.org/lkml/2a4200ac-3ea2-4449-94ac-c4b9f37ad800@kernel.org/#t
[2] https://lore.kernel.org/lkml/ed829240-d4f7-471f-84f6-3509f87f11a1@kernel.org/

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Krzysztof Kozlowski]

On 27/09/2024 09:59, Frank Wang wrote:
>>>
>>> -  clocks:
>>> -    maxItems: 1
>>> +  clocks: true
>>>
>>> -  clock-names:
>>> -    const: phyclk
>>> +  clock-names: true
>> For the third time, read the code I gave you. Do you see something like
>> this there? Why doing all the time something different than existing code?
> 
> Refer to the link you sent me that I must add minItems property for 
> clocks, just like the below codes:
> 
> @@ -35,7 +35,8 @@ properties:
>       const: 0
> 
>     clocks:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 3

Yes, for all variable properties, so also names.

> 
> That can pass dt_binding and dtb checking, however, "clocks" is the 
> optional property for some old Rockchip PHYs,  I am not sure is it right 
> to force set  minItems as 1 .
> If just keep maxItems, the dt_binding checking is failure.

Please specify the question you want to ask.


Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Krzysztof Kozlowski]

On 27/09/2024 10:02, Heiko Stuebner wrote:
> Hi Krzysztof,
> 
> Am Freitag, 27. September 2024, 09:30:30 CEST schrieb Krzysztof Kozlowski:
>> On 27/09/2024 09:01, Frank Wang wrote:
>>> Hi Krzysztof,
>>>
>>> On 2024/9/26 22:19, Krzysztof Kozlowski wrote:
>>>> On 26/09/2024 12:32, Frank Wang wrote:
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            enum:
>>>>> +              - rockchip,rk3576-usb2phy
>>>>> +    then:
>>>>> +      properties:
>>>>> +        clocks:
>>>>> +          minItems: 3
>>>>> +          maxItems: 3
>>>> Read one more time the example I gave you. Top-level constraints are
>>>> saying max one clock.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Sorry for overlooking this, I will set both "clocks" and "clock-names" 
>>> to true, and add the else case below the above codes for the "old" SoCs.
>>> Just like the below.
>>>
>>> -  clocks:
>>> -    maxItems: 1
>>> +  clocks: true
>>>
>>> -  clock-names:
>>> -    const: phyclk
>>> +  clock-names: true
>>
>> For the third time, read the code I gave you. Do you see something like
>> this there? Why doing all the time something different than existing code?
> 
> On vacation right now so late to the party, and somewhat confused :-) .
> 
> I've tried to find the code you mentioned, but did fail.
> In [0] you mention "maybe oneOf". The other replies in that version were
> about the ordering needing to stay for the older phy variants.
> 
> [1] in v2 has that NAK thing and [2] from v3 references that example again
> 
> I am probably just blind, but could use a pointer.

Oh, maybe I did not provide the link?

I apologize. I thought I gave reference to standard example. My bad.

Here it goes:
https://elixir.bootlin.com/linux/v6.11-rc6/source/Documentation/devicetree/bindings/ufs/qcom,ufs.yaml#L127


Best regards,
Krzysztof

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Frank Wang]

Hi Krzysztof,

On 2024/9/27 17:52, Krzysztof Kozlowski wrote:
> On 27/09/2024 09:59, Frank Wang wrote:
>>>> -  clocks:
>>>> -    maxItems: 1
>>>> +  clocks: true
>>>>
>>>> -  clock-names:
>>>> -    const: phyclk
>>>> +  clock-names: true
>>> For the third time, read the code I gave you. Do you see something like
>>> this there? Why doing all the time something different than existing code?
>> Refer to the link you sent me that I must add minItems property for
>> clocks, just like the below codes:
>>
>> @@ -35,7 +35,8 @@ properties:
>>        const: 0
>>
>>      clocks:
>> -    maxItems: 1
>> +    minItems: 1
>> +    maxItems: 3
> Yes, for all variable properties, so also names.
>
>> That can pass dt_binding and dtb checking, however, "clocks" is the
>> optional property for some old Rockchip PHYs,  I am not sure is it right
>> to force set  minItems as 1 .
>> If just keep maxItems, the dt_binding checking is failure.
> Please specify the question you want to ask.
>

Q1: The "clocks" is an optional property for some old Rockchip PHYs, so 
set "minItems: 1" likes the below, is this a suitable setting?

    clocks:
-    maxItems: 1
+    minItems: 1
+    maxItems: 3Q2: Do you want me to amend the code to like this? 
clocks: minItems: 1 maxItems: 3 clock-names: minItems: 1 maxItems: 3 
allOf: [...] - if: properties: compatible: contains: enum: - 
rockchip,px30-usb2phy - rockchip,rk3128-usb2phy - 
rockchip,rk3228-usb2phy - rockchip,rk3308-usb2phy - 
rockchip,rk3328-usb2phy - rockchip,rk3366-usb2phy - 
rockchip,rk3399-usb2phy - rockchip,rk3568-usb2phy - 
rockchip,rk3588-usb2phy - rockchip,rv1108-usb2phy then: properties: 
clocks: maxItems: 1 clock-names: const: phyclk - if: properties: 
compatible: contains: enum: - rockchip,rk3576-usb2phy then: properties: 
clocks: minItems: 3 maxItems: 3 clock-names: items: - const: phyclk - 
const: aclk - const: aclk_slv


Best regards,
Frank

> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Frank Wang]

Hi Krzysztof,

On 2024/9/27 17:52, Krzysztof Kozlowski wrote:
> On 27/09/2024 09:59, Frank Wang wrote:
>>>> -  clocks:
>>>> -    maxItems: 1
>>>> +  clocks: true
>>>>
>>>> -  clock-names:
>>>> -    const: phyclk
>>>> +  clock-names: true
>>> For the third time, read the code I gave you. Do you see something like
>>> this there? Why doing all the time something different than existing code?
>> Refer to the link you sent me that I must add minItems property for
>> clocks, just like the below codes:
>>
>> @@ -35,7 +35,8 @@ properties:
>>        const: 0
>>
>>      clocks:
>> -    maxItems: 1
>> +    minItems: 1
>> +    maxItems: 3
> Yes, for all variable properties, so also names.
>
>> That can pass dt_binding and dtb checking, however, "clocks" is the
>> optional property for some old Rockchip PHYs,  I am not sure is it right
>> to force set  minItems as 1 .
>> If just keep maxItems, the dt_binding checking is failure.
> Please specify the question you want to ask.
>

Please ignore my last garbled email as mailbox client issue.

Q1: The "clocks" is an optional property for some old Rockchip PHYs, so 
set "minItems: 1" likes the below, is this a suitable setting?

     clocks:
-    maxItems: 1
+    minItems: 1
+    maxItems: 3

Q2: Do you want me to amend the code to like this?

    clocks:
     minItems: 1
     maxItems: 3

    clock-names:
     minItems: 1
     maxItems: 3

allOf:

[...]

   - if:
       properties:
         compatible:
           contains:
             enum:
               - rockchip,px30-usb2phy
               - rockchip,rk3128-usb2phy
               - rockchip,rk3228-usb2phy
               - rockchip,rk3308-usb2phy
               - rockchip,rk3328-usb2phy
               - rockchip,rk3366-usb2phy
               - rockchip,rk3399-usb2phy
               - rockchip,rk3568-usb2phy
               - rockchip,rk3588-usb2phy
               - rockchip,rv1108-usb2phy
     then:
       properties:
         clocks:
           maxItems: 1
         clock-names:
           const: phyclk

   - if:
       properties:
         compatible:
           contains:
             enum:
               - rockchip,rk3576-usb2phy
     then:
       properties:
         clocks:
           minItems: 3
           maxItems: 3
         clock-names:
           items:
             - const: phyclk
             - const: aclk
             - const: aclk_slv

BR.
Frank

> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/2] dt-bindings: phy: rockchip,inno-usb2phy: add rk3576

[Krzysztof Kozlowski]

On 27/09/2024 12:59, Frank Wang wrote:
> Hi Krzysztof,
> 
> On 2024/9/27 17:52, Krzysztof Kozlowski wrote:
>> On 27/09/2024 09:59, Frank Wang wrote:
>>>>> -  clocks:
>>>>> -    maxItems: 1
>>>>> +  clocks: true
>>>>>
>>>>> -  clock-names:
>>>>> -    const: phyclk
>>>>> +  clock-names: true
>>>> For the third time, read the code I gave you. Do you see something like
>>>> this there? Why doing all the time something different than existing code?
>>> Refer to the link you sent me that I must add minItems property for
>>> clocks, just like the below codes:
>>>
>>> @@ -35,7 +35,8 @@ properties:
>>>        const: 0
>>>
>>>      clocks:
>>> -    maxItems: 1
>>> +    minItems: 1
>>> +    maxItems: 3
>> Yes, for all variable properties, so also names.
>>
>>> That can pass dt_binding and dtb checking, however, "clocks" is the
>>> optional property for some old Rockchip PHYs,  I am not sure is it right
>>> to force set  minItems as 1 .
>>> If just keep maxItems, the dt_binding checking is failure.
>> Please specify the question you want to ask.
>>
> 
> Please ignore my last garbled email as mailbox client issue.
> 
> Q1: The "clocks" is an optional property for some old Rockchip PHYs, so 
> set "minItems: 1" likes the below, is this a suitable setting?
> 
>      clocks:
> -    maxItems: 1
> +    minItems: 1
> +    maxItems: 3
> 
> Q2: Do you want me to amend the code to like this?
> 
>     clocks:
>      minItems: 1
>      maxItems: 3
> 
>     clock-names:
>      minItems: 1
>      maxItems: 3

List should be here with minItems. Then you only define the constraints
in if:then:

Best regards,
Krzysztof