linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/3] dts: imx8qxp add cdns usb3 port
@ 2023-03-16 21:27 Frank Li
  2023-03-16 21:27 ` [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings Frank Li
                   ` (2 more replies)
  0 siblings, 3 replies; 17+ messages in thread
From: Frank Li @ 2023-03-16 21:27 UTC (permalink / raw)
  To: shawnguo
  Cc: Frank.Li, devicetree, festevam, imx, kernel,
	krzysztof.kozlowski+dt, linux-arm-kernel, linux-imx,
	linux-kernel, robh+dt, s.hauer

cdns driver code already upstreamed. but missed dts part.

Change from v1 to v2:
1. Add binding docoument.
2. Fixed all shawn's comments

Frank Li (3):
  dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  arm64: dts: imx8qxp: add cadence usb3 support
  arm64: dts: freescale: imx8qxp-mek: enable cadence usb3

 .../bindings/usb/fsl,imx8qm-cdns3.yaml        | 122 ++++++++++++++++++
 .../boot/dts/freescale/imx8-ss-conn.dtsi      |  72 +++++++++++
 arch/arm64/boot/dts/freescale/imx8qxp-mek.dts |  85 ++++++++++++
 3 files changed, 279 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml

-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-16 21:27 [PATCH v2 0/3] dts: imx8qxp add cdns usb3 port Frank Li
@ 2023-03-16 21:27 ` Frank Li
  2023-03-17  9:09   ` Krzysztof Kozlowski
  2023-03-16 21:27 ` [PATCH v2 2/3] arm64: dts: imx8qxp: add cadence usb3 support Frank Li
  2023-03-16 21:27 ` [PATCH v2 3/3] arm64: dts: freescale: imx8qxp-mek: enable cadence usb3 Frank Li
  2 siblings, 1 reply; 17+ messages in thread
From: Frank Li @ 2023-03-16 21:27 UTC (permalink / raw)
  To: shawnguo
  Cc: Frank.Li, devicetree, festevam, imx, kernel,
	krzysztof.kozlowski+dt, linux-arm-kernel, linux-imx,
	linux-kernel, robh+dt, s.hauer

NXP imx8qm integrates 1 cdns3 IP. This is glue layer device bindings.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../bindings/usb/fsl,imx8qm-cdns3.yaml        | 122 ++++++++++++++++++
 1 file changed, 122 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml

diff --git a/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml b/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml
new file mode 100644
index 000000000000..fc24df1e4483
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml
@@ -0,0 +1,122 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (c) 2020 NXP
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/fsl,imx8qm-cdns3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: NXP iMX8QM Soc USB Controller
+
+maintainers:
+  - Frank Li <Frank.Li@nxp.com>
+
+properties:
+  compatible:
+    const: fsl,imx8qm-usb3
+
+  reg:
+    items:
+      - description: Address and length of the register set for iMX USB3 Platform Control
+
+  "#address-cells":
+    enum: [ 1, 2 ]
+
+  "#size-cells":
+    enum: [ 1, 2 ]
+
+  ranges: true
+
+  clocks:
+    description:
+      A list of phandle and clock-specifier pairs for the clocks
+      listed in clock-names.
+    items:
+      - description: Standby clock. Used during ultra low power states.
+      - description: USB bus clock for usb3 controller.
+      - description: AXI clock for AXI interface.
+      - description: ipg clock for register access.
+      - description: Core clock for usb3 controller.
+
+  clock-names:
+    items:
+      - const: usb3_lpm_clk
+      - const: usb3_bus_clk
+      - const: usb3_aclk
+      - const: usb3_ipg_clk
+      - const: usb3_core_pclk
+
+  assigned-clocks:
+    items:
+      - description: Phandle and clock specifier of IMX_SC_PM_CLK_PER.
+      - description: Phandle and clock specifoer of IMX_SC_PM_CLK_MISC.
+      - description: Phandle and clock specifoer of IMX_SC_PM_CLK_MST_BUS.
+
+  assigned-clock-rates:
+    items:
+      - description: Must be 125 Mhz.
+      - description: Must be 12 Mhz.
+      - description: Must be 250 Mhz.
+
+  power-domains:
+    maxItems: 1
+
+# Required child node:
+
+patternProperties:
+  "^usb@[0-9a-f]+$":
+    $ref: cdns,usb3.yaml#
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+  - clocks
+  - clock-names
+  - power-domains
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx8-lpcg.h>
+    #include <dt-bindings/firmware/imx/rsrc.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    usbotg3: usb@5b110000 {
+      compatible = "fsl,imx8qm-usb3";
+      ranges;
+      reg = <0x5b110000 0x10000>;
+      clocks = <&usb3_lpcg IMX_LPCG_CLK_1>,
+               <&usb3_lpcg IMX_LPCG_CLK_0>,
+               <&usb3_lpcg IMX_LPCG_CLK_7>,
+               <&usb3_lpcg IMX_LPCG_CLK_4>,
+               <&usb3_lpcg IMX_LPCG_CLK_5>;
+      clock-names = "usb3_lpm_clk", "usb3_bus_clk", "usb3_aclk",
+                    "usb3_ipg_clk", "usb3_core_pclk";
+      assigned-clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
+                        <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
+                        <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MST_BUS>;
+      assigned-clock-rates = <125000000>, <12000000>, <250000000>;
+      power-domains = <&pd IMX_SC_R_USB_2>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+      status = "disabled";
+
+      usbotg3_cdns3: usb@5b120000 {
+          compatible = "cdns,usb3";
+          interrupt-parent = <&gic>;
+          interrupts = <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
+                       <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>;
+          interrupt-names = "host", "peripheral", "otg", "wakeup";
+          reg = <0x5b120000 0x10000>,   /* memory area for OTG/DRD registers */
+                <0x5b130000 0x10000>,   /* memory area for HOST registers */
+                <0x5b140000 0x10000>;   /* memory area for DEVICE registers */
+          reg-names = "otg", "xhci", "dev";
+          phys = <&usb3_phy>;
+          phy-names = "cdns3,usb3-phy";
+      };
+    };
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 2/3] arm64: dts: imx8qxp: add cadence usb3 support
  2023-03-16 21:27 [PATCH v2 0/3] dts: imx8qxp add cdns usb3 port Frank Li
  2023-03-16 21:27 ` [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings Frank Li
@ 2023-03-16 21:27 ` Frank Li
  2023-03-17  6:39   ` Alexander Stein
  2023-03-16 21:27 ` [PATCH v2 3/3] arm64: dts: freescale: imx8qxp-mek: enable cadence usb3 Frank Li
  2 siblings, 1 reply; 17+ messages in thread
From: Frank Li @ 2023-03-16 21:27 UTC (permalink / raw)
  To: shawnguo
  Cc: Frank.Li, devicetree, festevam, imx, kernel,
	krzysztof.kozlowski+dt, linux-arm-kernel, linux-imx,
	linux-kernel, robh+dt, s.hauer

There are cadence usb3.0 controller in 8qxp and 8qm.
Add usb3 node at common connect subsystem.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 .../boot/dts/freescale/imx8-ss-conn.dtsi      | 72 +++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi b/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi
index 4852760adeee..389f52f16a5c 100644
--- a/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi
@@ -138,6 +138,56 @@ fec2: ethernet@5b050000 {
 		status = "disabled";
 	};
 
+	usbotg3: usb@5b110000 {
+		compatible = "fsl,imx8qm-usb3";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges;
+		reg = <0x5b110000 0x10000>;
+		clocks = <&usb3_lpcg IMX_LPCG_CLK_1>,
+			 <&usb3_lpcg IMX_LPCG_CLK_0>,
+			 <&usb3_lpcg IMX_LPCG_CLK_7>,
+			 <&usb3_lpcg IMX_LPCG_CLK_4>,
+			 <&usb3_lpcg IMX_LPCG_CLK_5>;
+		clock-names = "usb3_lpm_clk", "usb3_bus_clk", "usb3_aclk",
+			      "usb3_ipg_clk", "usb3_core_pclk";
+		assigned-clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
+				  <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
+				  <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MST_BUS>;
+		assigned-clock-rates = <125000000>, <12000000>, <250000000>;
+		power-domains = <&pd IMX_SC_R_USB_2>;
+		status = "disabled";
+
+		usbotg3_cdns3: usb@5b120000 {
+			compatible = "cdns,usb3";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			interrupt-parent = <&gic>;
+			interrupts = <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "host", "peripheral", "otg", "wakeup";
+			reg = <0x5b130000 0x10000>,     /* memory area for HOST registers */
+			      <0x5b140000 0x10000>,   /* memory area for DEVICE registers */
+			      <0x5b120000 0x10000>;   /* memory area for OTG/DRD registers */
+			reg-names = "xhci", "dev", "otg";
+			phys = <&usb3_phy>;
+			phy-names = "cdns3,usb3-phy";
+			status = "disabled";
+		};
+	};
+
+	usb3_phy: usb-phy@5b160000 {
+		compatible = "nxp,salvo-phy";
+		reg = <0x5b160000 0x40000>;
+		clocks = <&usb3_lpcg IMX_LPCG_CLK_6>;
+		clock-names = "salvo_phy_clk";
+		power-domains = <&pd IMX_SC_R_USB_2_PHY>;
+		#phy-cells = <0>;
+		status = "disabled";
+	};
+
 	/* LPCG clocks */
 	sdhc0_lpcg: clock-controller@5b200000 {
 		compatible = "fsl,imx8qxp-lpcg";
@@ -234,4 +284,26 @@ usb2_lpcg: clock-controller@5b270000 {
 		clock-output-names = "usboh3_ahb_clk", "usboh3_phy_ipg_clk";
 		power-domains = <&pd IMX_SC_R_USB_0_PHY>;
 	};
+
+	usb3_lpcg: clock-controller@5b280000 {
+		compatible = "fsl,imx8qxp-lpcg";
+		reg = <0x5b280000 0x10000>;
+		#clock-cells = <1>;
+		clock-indices = <IMX_LPCG_CLK_0>, <IMX_LPCG_CLK_1>,
+				<IMX_LPCG_CLK_4>, <IMX_LPCG_CLK_5>,
+				<IMX_LPCG_CLK_6>, <IMX_LPCG_CLK_7>;
+		clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
+			 <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
+			 <&conn_ipg_clk>,
+			 <&conn_ipg_clk>,
+			 <&conn_ipg_clk>,
+			 <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MST_BUS>;
+		clock-output-names = "usb3_app_clk",
+				     "usb3_lpm_clk",
+				     "usb3_ipg_clk",
+				     "usb3_core_pclk",
+				     "usb3_phy_clk",
+				     "usb3_aclk";
+		power-domains = <&pd IMX_SC_R_USB_2_PHY>;
+	};
 };
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH v2 3/3] arm64: dts: freescale: imx8qxp-mek: enable cadence usb3
  2023-03-16 21:27 [PATCH v2 0/3] dts: imx8qxp add cdns usb3 port Frank Li
  2023-03-16 21:27 ` [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings Frank Li
  2023-03-16 21:27 ` [PATCH v2 2/3] arm64: dts: imx8qxp: add cadence usb3 support Frank Li
@ 2023-03-16 21:27 ` Frank Li
  2 siblings, 0 replies; 17+ messages in thread
From: Frank Li @ 2023-03-16 21:27 UTC (permalink / raw)
  To: shawnguo
  Cc: Frank.Li, devicetree, festevam, imx, kernel,
	krzysztof.kozlowski+dt, linux-arm-kernel, linux-imx,
	linux-kernel, robh+dt, s.hauer

Enable USB3 controller, phy and typec related nodes.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 arch/arm64/boot/dts/freescale/imx8qxp-mek.dts | 85 +++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
index afa883389456..9ba4c72f0006 100644
--- a/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
+++ b/arch/arm64/boot/dts/freescale/imx8qxp-mek.dts
@@ -6,6 +6,7 @@
 /dts-v1/;
 
 #include "imx8qxp.dtsi"
+#include <dt-bindings/usb/pd.h>
 
 / {
 	model = "Freescale i.MX8QXP MEK";
@@ -28,6 +29,21 @@ reg_usdhc2_vmmc: usdhc2-vmmc {
 		gpio = <&lsio_gpio4 19 GPIO_ACTIVE_HIGH>;
 		enable-active-high;
 	};
+
+	gpio-sbu-mux {
+		compatible = "gpio-sbu-mux";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_typec_mux>;
+		select-gpios = <&lsio_gpio5 9 GPIO_ACTIVE_HIGH>;
+		enable-gpios = <&pca9557_a 7 GPIO_ACTIVE_LOW>;
+		orientation-switch;
+
+		port {
+			usb3_data_ss: endpoint {
+				remote-endpoint = <&typec_con_ss>;
+			};
+		};
+	};
 };
 
 &dsp {
@@ -127,6 +143,42 @@ light-sensor@44 {
 			};
 		};
 	};
+
+	ptn5110: tcpc@50 {
+		compatible = "nxp,ptn5110";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_typec>;
+		reg = <0x50>;
+		interrupt-parent = <&lsio_gpio1>;
+		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+
+		port {
+			typec_dr_sw: endpoint {
+				remote-endpoint = <&usb3_drd_sw>;
+			};
+		};
+
+		usb_con1: connector {
+			compatible = "usb-c-connector";
+			label = "USB-C";
+			power-role = "source";
+			data-role = "dual";
+			source-pdos = <PDO_FIXED(5000, 3000, PDO_FIXED_USB_COMM)>;
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@1 {
+					reg = <1>;
+					typec_con_ss: endpoint {
+						remote-endpoint = <&usb3_data_ss>;
+					};
+				};
+			};
+		};
+	};
+
 };
 
 &lpuart0 {
@@ -204,6 +256,27 @@ &usdhc2 {
 	status = "okay";
 };
 
+&usb3_phy {
+	status = "okay";
+};
+
+&usbotg3 {
+	status = "okay";
+};
+
+&usbotg3_cdns3 {
+	dr_mode = "otg";
+	usb-role-switch;
+	status = "okay";
+
+	port {
+		usb3_drd_sw: endpoint {
+			remote-endpoint = <&typec_dr_sw>;
+		};
+	};
+};
+
+
 &vpu {
 	compatible = "nxp,imx8qxp-vpu";
 	status = "okay";
@@ -267,6 +340,18 @@ IMX8QXP_UART0_TX_ADMA_UART0_TX				0x06000020
 		>;
 	};
 
+	pinctrl_typec: typecgrp {
+		fsl,pins = <
+			IMX8QXP_SPI2_SCK_LSIO_GPIO1_IO03                        0x06000021
+		>;
+	};
+
+	pinctrl_typec_mux: typecmuxgrp {
+		fsl,pins = <
+			IMX8QXP_ENET0_REFCLK_125M_25M_LSIO_GPIO5_IO09           0x60
+		>;
+	};
+
 	pinctrl_usdhc1: usdhc1grp {
 		fsl,pins = <
 			IMX8QXP_EMMC0_CLK_CONN_EMMC0_CLK			0x06000041
-- 
2.34.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 2/3] arm64: dts: imx8qxp: add cadence usb3 support
  2023-03-16 21:27 ` [PATCH v2 2/3] arm64: dts: imx8qxp: add cadence usb3 support Frank Li
@ 2023-03-17  6:39   ` Alexander Stein
  2023-03-17 15:05     ` [EXT] " Frank Li
  0 siblings, 1 reply; 17+ messages in thread
From: Alexander Stein @ 2023-03-17  6:39 UTC (permalink / raw)
  To: shawnguo, linux-arm-kernel
  Cc: Frank.Li, devicetree, festevam, imx, kernel,
	krzysztof.kozlowski+dt, linux-arm-kernel, linux-imx,
	linux-kernel, robh+dt, s.hauer, Frank Li

Am Donnerstag, 16. März 2023, 22:27:10 CET schrieb Frank Li:
> There are cadence usb3.0 controller in 8qxp and 8qm.
> Add usb3 node at common connect subsystem.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  .../boot/dts/freescale/imx8-ss-conn.dtsi      | 72 +++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi
> b/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi index
> 4852760adeee..389f52f16a5c 100644
> --- a/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi
> @@ -138,6 +138,56 @@ fec2: ethernet@5b050000 {
>  		status = "disabled";
>  	};
> 
> +	usbotg3: usb@5b110000 {
> +		compatible = "fsl,imx8qm-usb3";

Mh, is imx8qm considered a subset of imx8qxp or vice versa?
Maybe it's worth adding a dedicated compatible for imx8qxp as well.

Best regards,
Alexander

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges;
> +		reg = <0x5b110000 0x10000>;
> +		clocks = <&usb3_lpcg IMX_LPCG_CLK_1>,
> +			 <&usb3_lpcg IMX_LPCG_CLK_0>,
> +			 <&usb3_lpcg IMX_LPCG_CLK_7>,
> +			 <&usb3_lpcg IMX_LPCG_CLK_4>,
> +			 <&usb3_lpcg IMX_LPCG_CLK_5>;
> +		clock-names = "usb3_lpm_clk", "usb3_bus_clk", "usb3_aclk",
> +			      "usb3_ipg_clk", "usb3_core_pclk";
> +		assigned-clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
> +				  <&clk IMX_SC_R_USB_2 
IMX_SC_PM_CLK_MISC>,
> +				  <&clk IMX_SC_R_USB_2 
IMX_SC_PM_CLK_MST_BUS>;
> +		assigned-clock-rates = <125000000>, <12000000>, 
<250000000>;
> +		power-domains = <&pd IMX_SC_R_USB_2>;
> +		status = "disabled";
> +
> +		usbotg3_cdns3: usb@5b120000 {
> +			compatible = "cdns,usb3";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			interrupt-parent = <&gic>;
> +			interrupts = <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 271 
IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 271 
IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 271 
IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "host", "peripheral", "otg", 
"wakeup";
> +			reg = <0x5b130000 0x10000>,     /* memory area 
for HOST registers */
> +			      <0x5b140000 0x10000>,   /* memory area for 
DEVICE registers */
> +			      <0x5b120000 0x10000>;   /* memory area for 
OTG/DRD registers */
> +			reg-names = "xhci", "dev", "otg";
> +			phys = <&usb3_phy>;
> +			phy-names = "cdns3,usb3-phy";
> +			status = "disabled";
> +		};
> +	};
> +
> +	usb3_phy: usb-phy@5b160000 {
> +		compatible = "nxp,salvo-phy";
> +		reg = <0x5b160000 0x40000>;
> +		clocks = <&usb3_lpcg IMX_LPCG_CLK_6>;
> +		clock-names = "salvo_phy_clk";
> +		power-domains = <&pd IMX_SC_R_USB_2_PHY>;
> +		#phy-cells = <0>;
> +		status = "disabled";
> +	};
> +
>  	/* LPCG clocks */
>  	sdhc0_lpcg: clock-controller@5b200000 {
>  		compatible = "fsl,imx8qxp-lpcg";
> @@ -234,4 +284,26 @@ usb2_lpcg: clock-controller@5b270000 {
>  		clock-output-names = "usboh3_ahb_clk", 
"usboh3_phy_ipg_clk";
>  		power-domains = <&pd IMX_SC_R_USB_0_PHY>;
>  	};
> +
> +	usb3_lpcg: clock-controller@5b280000 {
> +		compatible = "fsl,imx8qxp-lpcg";
> +		reg = <0x5b280000 0x10000>;
> +		#clock-cells = <1>;
> +		clock-indices = <IMX_LPCG_CLK_0>, <IMX_LPCG_CLK_1>,
> +				<IMX_LPCG_CLK_4>, <IMX_LPCG_CLK_5>,
> +				<IMX_LPCG_CLK_6>, <IMX_LPCG_CLK_7>;
> +		clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
> +			 <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
> +			 <&conn_ipg_clk>,
> +			 <&conn_ipg_clk>,
> +			 <&conn_ipg_clk>,
> +			 <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MST_BUS>;
> +		clock-output-names = "usb3_app_clk",
> +				     "usb3_lpm_clk",
> +				     "usb3_ipg_clk",
> +				     "usb3_core_pclk",
> +				     "usb3_phy_clk",
> +				     "usb3_aclk";
> +		power-domains = <&pd IMX_SC_R_USB_2_PHY>;
> +	};
>  };


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-16 21:27 ` [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings Frank Li
@ 2023-03-17  9:09   ` Krzysztof Kozlowski
  2023-03-17 14:55     ` [EXT] " Frank Li
  0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-17  9:09 UTC (permalink / raw)
  To: Frank Li, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, linux-imx, linux-kernel, robh+dt, s.hauer

On 16/03/2023 22:27, Frank Li wrote:
> NXP imx8qm integrates 1 cdns3 IP. This is glue layer device bindings.
> 

Subject: drop second/last, redundant "bindings". The "dt-bindings"
prefix is already stating that these are bindings.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  .../bindings/usb/fsl,imx8qm-cdns3.yaml        | 122 ++++++++++++++++++
>  1 file changed, 122 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml
> 
> diff --git a/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml b/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml
> new file mode 100644
> index 000000000000..fc24df1e4483
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml
> @@ -0,0 +1,122 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright (c) 2020 NXP
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/usb/fsl,imx8qm-cdns3.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP iMX8QM Soc USB Controller
> +
> +maintainers:
> +  - Frank Li <Frank.Li@nxp.com>
> +
> +properties:
> +  compatible:
> +    const: fsl,imx8qm-usb3
> +
> +  reg:
> +    items:
> +      - description: Address and length of the register set for iMX USB3 Platform Control

Drop "Address and length of the"... or actually just maxItems: 1,
because the description is a bit obvious.

> +
> +  "#address-cells":
> +    enum: [ 1, 2 ]
> +
> +  "#size-cells":
> +    enum: [ 1, 2 ]
> +
> +  ranges: true
> +
> +  clocks:
> +    description:
> +      A list of phandle and clock-specifier pairs for the clocks
> +      listed in clock-names.

Drop description.

> +    items:
> +      - description: Standby clock. Used during ultra low power states.
> +      - description: USB bus clock for usb3 controller.
> +      - description: AXI clock for AXI interface.
> +      - description: ipg clock for register access.
> +      - description: Core clock for usb3 controller.
> +
> +  clock-names:
> +    items:
> +      - const: usb3_lpm_clk
> +      - const: usb3_bus_clk
> +      - const: usb3_aclk
> +      - const: usb3_ipg_clk
> +      - const: usb3_core_pclk
> +
> +  assigned-clocks:
> +    items:
> +      - description: Phandle and clock specifier of IMX_SC_PM_CLK_PER.
> +      - description: Phandle and clock specifoer of IMX_SC_PM_CLK_MISC.
> +      - description: Phandle and clock specifoer of IMX_SC_PM_CLK_MST_BUS.
> +
> +  assigned-clock-rates:
> +    items:
> +      - description: Must be 125 Mhz.
> +      - description: Must be 12 Mhz.
> +      - description: Must be 250 Mhz.

I would argue that both properties above are not needed. If your
hardware requires fixed frequencies, clock provider can fix them, can't it?

> +
> +  power-domains:
> +    maxItems: 1
> +
> +# Required child node:
> +
> +patternProperties:
> +  "^usb@[0-9a-f]+$":
> +    $ref: cdns,usb3.yaml#
> +
> +required:
> +  - compatible
> +  - reg
> +  - "#address-cells"
> +  - "#size-cells"
> +  - ranges
> +  - clocks
> +  - clock-names
> +  - power-domains
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/imx8-lpcg.h>
> +    #include <dt-bindings/firmware/imx/rsrc.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    usbotg3: usb@5b110000 {

Drop label, unused

> +      compatible = "fsl,imx8qm-usb3";
> +      ranges;
> +      reg = <0x5b110000 0x10000>;

reg is second property

> +      clocks = <&usb3_lpcg IMX_LPCG_CLK_1>,
> +               <&usb3_lpcg IMX_LPCG_CLK_0>,
> +               <&usb3_lpcg IMX_LPCG_CLK_7>,
> +               <&usb3_lpcg IMX_LPCG_CLK_4>,
> +               <&usb3_lpcg IMX_LPCG_CLK_5>;
> +      clock-names = "usb3_lpm_clk", "usb3_bus_clk", "usb3_aclk",
> +                    "usb3_ipg_clk", "usb3_core_pclk";
> +      assigned-clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
> +                        <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
> +                        <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MST_BUS>;
> +      assigned-clock-rates = <125000000>, <12000000>, <250000000>;
> +      power-domains = <&pd IMX_SC_R_USB_2>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      status = "disabled";

Drop status

> +
> +      usbotg3_cdns3: usb@5b120000 {

Drop label

> +          compatible = "cdns,usb3";
> +          interrupt-parent = <&gic>;
> +          interrupts = <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
> +                       <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>;
> +          interrupt-names = "host", "peripheral", "otg", "wakeup";
> +          reg = <0x5b120000 0x10000>,   /* memory area for OTG/DRD registers */
> +                <0x5b130000 0x10000>,   /* memory area for HOST registers */
> +                <0x5b140000 0x10000>;   /* memory area for DEVICE registers */
> +          reg-names = "otg", "xhci", "dev";

reg is second property, reg-names third.

> +          phys = <&usb3_phy>;
> +          phy-names = "cdns3,usb3-phy";
> +      };
> +    };

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-17  9:09   ` Krzysztof Kozlowski
@ 2023-03-17 14:55     ` Frank Li
  2023-03-19 11:13       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 17+ messages in thread
From: Frank Li @ 2023-03-17 14:55 UTC (permalink / raw)
  To: Krzysztof Kozlowski, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer



> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Friday, March 17, 2023 4:09 AM
> To: Frank Li <frank.li@nxp.com>; shawnguo@kernel.org
> Cc: devicetree@vger.kernel.org; festevam@gmail.com; imx@lists.linux.dev;
> kernel@pengutronix.de; krzysztof.kozlowski+dt@linaro.org; linux-arm-
> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; linux-
> kernel@vger.kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
> Subject: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add
> imx8qm cdns3 glue bindings
> 
> Caution: EXT Email
> 
> On 16/03/2023 22:27, Frank Li wrote:
> > NXP imx8qm integrates 1 cdns3 IP. This is glue layer device bindings.
> >
> 
> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> prefix is already stating that these are bindings.
> 
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  .../bindings/usb/fsl,imx8qm-cdns3.yaml        | 122 ++++++++++++++++++
> >  1 file changed, 122 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/fsl,imx8qm-
> cdns3.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/usb/fsl,imx8qm-
> cdns3.yaml b/Documentation/devicetree/bindings/usb/fsl,imx8qm-
> cdns3.yaml
> > new file mode 100644
> > index 000000000000..fc24df1e4483
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml
> > @@ -0,0 +1,122 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright (c) 2020 NXP
> > +%YAML 1.2
> > +---
> > +$id:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
> tree.org%2Fschemas%2Fusb%2Ffsl%2Cimx8qm-
> cdns3.yaml%23&data=05%7C01%7CFrank.Li%40nxp.com%7Cac9af3d617dc4cf
> 14baf08db26c74b07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> 38146409617970248%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C
> &sdata=EczZhjyMUGnnp7ZGfSvTj4lmOUuGlOtWYIsxxXIlNXw%3D&reserved
> =0
> > +$schema:
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
> tree.org%2Fmeta-
> schemas%2Fcore.yaml%23&data=05%7C01%7CFrank.Li%40nxp.com%7Cac9a
> f3d617dc4cf14baf08db26c74b07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> 0%7C0%7C638146409617970248%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
> C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> %7C%7C%7C&sdata=uTNYuDm%2ByhZ56oQET2pX8sHpEqVvsUYtmOBCPXEK
> v40%3D&reserved=0
> > +
> > +title: NXP iMX8QM Soc USB Controller
> > +
> > +maintainers:
> > +  - Frank Li <Frank.Li@nxp.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: fsl,imx8qm-usb3
> > +
> > +  reg:
> > +    items:
> > +      - description: Address and length of the register set for iMX USB3
> Platform Control
> 
> Drop "Address and length of the"... or actually just maxItems: 1,
> because the description is a bit obvious.
> 
> > +
> > +  "#address-cells":
> > +    enum: [ 1, 2 ]
> > +
> > +  "#size-cells":
> > +    enum: [ 1, 2 ]
> > +
> > +  ranges: true
> > +
> > +  clocks:
> > +    description:
> > +      A list of phandle and clock-specifier pairs for the clocks
> > +      listed in clock-names.
> 
> Drop description.
> 
> > +    items:
> > +      - description: Standby clock. Used during ultra low power states.
> > +      - description: USB bus clock for usb3 controller.
> > +      - description: AXI clock for AXI interface.
> > +      - description: ipg clock for register access.
> > +      - description: Core clock for usb3 controller.
> > +
> > +  clock-names:
> > +    items:
> > +      - const: usb3_lpm_clk
> > +      - const: usb3_bus_clk
> > +      - const: usb3_aclk
> > +      - const: usb3_ipg_clk
> > +      - const: usb3_core_pclk
> > +
> > +  assigned-clocks:
> > +    items:
> > +      - description: Phandle and clock specifier of IMX_SC_PM_CLK_PER.
> > +      - description: Phandle and clock specifoer of IMX_SC_PM_CLK_MISC.
> > +      - description: Phandle and clock specifoer of
> IMX_SC_PM_CLK_MST_BUS.
> > +
> > +  assigned-clock-rates:
> > +    items:
> > +      - description: Must be 125 Mhz.
> > +      - description: Must be 12 Mhz.
> > +      - description: Must be 250 Mhz.
> 
> I would argue that both properties above are not needed. If your
> hardware requires fixed frequencies, clock provider can fix them, can't it?

Clock provider don't know fixed value and turn on only used by client. 

> 
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +# Required child node:
> > +
> > +patternProperties:
> > +  "^usb@[0-9a-f]+$":
> > +    $ref: cdns,usb3.yaml#
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - "#address-cells"
> > +  - "#size-cells"
> > +  - ranges
> > +  - clocks
> > +  - clock-names
> > +  - power-domains
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/imx8-lpcg.h>
> > +    #include <dt-bindings/firmware/imx/rsrc.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    usbotg3: usb@5b110000 {
> 
> Drop label, unused
> 
> > +      compatible = "fsl,imx8qm-usb3";
> > +      ranges;
> > +      reg = <0x5b110000 0x10000>;
> 
> reg is second property
> 
> > +      clocks = <&usb3_lpcg IMX_LPCG_CLK_1>,
> > +               <&usb3_lpcg IMX_LPCG_CLK_0>,
> > +               <&usb3_lpcg IMX_LPCG_CLK_7>,
> > +               <&usb3_lpcg IMX_LPCG_CLK_4>,
> > +               <&usb3_lpcg IMX_LPCG_CLK_5>;
> > +      clock-names = "usb3_lpm_clk", "usb3_bus_clk", "usb3_aclk",
> > +                    "usb3_ipg_clk", "usb3_core_pclk";
> > +      assigned-clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
> > +                        <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
> > +                        <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MST_BUS>;
> > +      assigned-clock-rates = <125000000>, <12000000>, <250000000>;
> > +      power-domains = <&pd IMX_SC_R_USB_2>;
> > +      #address-cells = <1>;
> > +      #size-cells = <1>;
> > +      status = "disabled";
> 
> Drop status
> 
> > +
> > +      usbotg3_cdns3: usb@5b120000 {
> 
> Drop label
> 
> > +          compatible = "cdns,usb3";
> > +          interrupt-parent = <&gic>;
> > +          interrupts = <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>,
> > +                       <GIC_SPI 271 IRQ_TYPE_LEVEL_HIGH>;
> > +          interrupt-names = "host", "peripheral", "otg", "wakeup";
> > +          reg = <0x5b120000 0x10000>,   /* memory area for OTG/DRD
> registers */
> > +                <0x5b130000 0x10000>,   /* memory area for HOST registers */
> > +                <0x5b140000 0x10000>;   /* memory area for DEVICE registers */
> > +          reg-names = "otg", "xhci", "dev";
> 
> reg is second property, reg-names third.
> 
> > +          phys = <&usb3_phy>;
> > +          phy-names = "cdns3,usb3-phy";
> > +      };
> > +    };
> 
> Best regards,
> Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v2 2/3] arm64: dts: imx8qxp: add cadence usb3 support
  2023-03-17  6:39   ` Alexander Stein
@ 2023-03-17 15:05     ` Frank Li
  0 siblings, 0 replies; 17+ messages in thread
From: Frank Li @ 2023-03-17 15:05 UTC (permalink / raw)
  To: Alexander Stein, shawnguo, linux-arm-kernel
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer

> 
> Caution: EXT Email
> 
> Am Donnerstag, 16. März 2023, 22:27:10 CET schrieb Frank Li:
> > There are cadence usb3.0 controller in 8qxp and 8qm.
> > Add usb3 node at common connect subsystem.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  .../boot/dts/freescale/imx8-ss-conn.dtsi      | 72 +++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi index
> > 4852760adeee..389f52f16a5c 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8-ss-conn.dtsi
> > @@ -138,6 +138,56 @@ fec2: ethernet@5b050000 {
> >               status = "disabled";
> >       };
> >
> > +     usbotg3: usb@5b110000 {
> > +             compatible = "fsl,imx8qm-usb3";
> 
> Mh, is imx8qm considered a subset of imx8qxp or vice versa?
> Maybe it's worth adding a dedicated compatible for imx8qxp as well.

Imx8qxp is subset of imx8qm.
This part is the same between qxp and qm. 
If exact same,  I think not necessary to add compatible string. 

> 
> Best regards,
> Alexander
> 
> > +             #address-cells = <1>;
> > +             #size-cells = <1>;
> > +             ranges;
> > +             reg = <0x5b110000 0x10000>;
> > +             clocks = <&usb3_lpcg IMX_LPCG_CLK_1>,
> > +                      <&usb3_lpcg IMX_LPCG_CLK_0>,
> > +                      <&usb3_lpcg IMX_LPCG_CLK_7>,
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-17 14:55     ` [EXT] " Frank Li
@ 2023-03-19 11:13       ` Krzysztof Kozlowski
  2023-03-20 14:49         ` Frank Li
  0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-19 11:13 UTC (permalink / raw)
  To: Frank Li, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer

On 17/03/2023 15:55, Frank Li wrote:
> 
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> Sent: Friday, March 17, 2023 4:09 AM
>> To: Frank Li <frank.li@nxp.com>; shawnguo@kernel.org
>> Cc: devicetree@vger.kernel.org; festevam@gmail.com; imx@lists.linux.dev;
>> kernel@pengutronix.de; krzysztof.kozlowski+dt@linaro.org; linux-arm-
>> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; linux-
>> kernel@vger.kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
>> Subject: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add
>> imx8qm cdns3 glue bindings
>>
>> Caution: EXT Email
>>
>> On 16/03/2023 22:27, Frank Li wrote:
>>> NXP imx8qm integrates 1 cdns3 IP. This is glue layer device bindings.
>>>
>>
>> Subject: drop second/last, redundant "bindings". The "dt-bindings"
>> prefix is already stating that these are bindings.
>>
>>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
>>> ---
>>>  .../bindings/usb/fsl,imx8qm-cdns3.yaml        | 122 ++++++++++++++++++
>>>  1 file changed, 122 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/usb/fsl,imx8qm-
>> cdns3.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/fsl,imx8qm-
>> cdns3.yaml b/Documentation/devicetree/bindings/usb/fsl,imx8qm-
>> cdns3.yaml
>>> new file mode 100644
>>> index 000000000000..fc24df1e4483
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml
>>> @@ -0,0 +1,122 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +# Copyright (c) 2020 NXP
>>> +%YAML 1.2
>>> +---
>>> +$id:
>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
>> tree.org%2Fschemas%2Fusb%2Ffsl%2Cimx8qm-
>> cdns3.yaml%23&data=05%7C01%7CFrank.Li%40nxp.com%7Cac9af3d617dc4cf
>> 14baf08db26c74b07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
>> 38146409617970248%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
>> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C
>> &sdata=EczZhjyMUGnnp7ZGfSvTj4lmOUuGlOtWYIsxxXIlNXw%3D&reserved
>> =0
>>> +$schema:
>> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
>> tree.org%2Fmeta-
>> schemas%2Fcore.yaml%23&data=05%7C01%7CFrank.Li%40nxp.com%7Cac9a
>> f3d617dc4cf14baf08db26c74b07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
>> 0%7C0%7C638146409617970248%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
>> C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
>> %7C%7C%7C&sdata=uTNYuDm%2ByhZ56oQET2pX8sHpEqVvsUYtmOBCPXEK
>> v40%3D&reserved=0
>>> +
>>> +title: NXP iMX8QM Soc USB Controller
>>> +
>>> +maintainers:
>>> +  - Frank Li <Frank.Li@nxp.com>
>>> +
>>> +properties:
>>> +  compatible:
>>> +    const: fsl,imx8qm-usb3
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: Address and length of the register set for iMX USB3
>> Platform Control
>>
>> Drop "Address and length of the"... or actually just maxItems: 1,
>> because the description is a bit obvious.
>>
>>> +
>>> +  "#address-cells":
>>> +    enum: [ 1, 2 ]
>>> +
>>> +  "#size-cells":
>>> +    enum: [ 1, 2 ]
>>> +
>>> +  ranges: true
>>> +
>>> +  clocks:
>>> +    description:
>>> +      A list of phandle and clock-specifier pairs for the clocks
>>> +      listed in clock-names.
>>
>> Drop description.
>>
>>> +    items:
>>> +      - description: Standby clock. Used during ultra low power states.
>>> +      - description: USB bus clock for usb3 controller.
>>> +      - description: AXI clock for AXI interface.
>>> +      - description: ipg clock for register access.
>>> +      - description: Core clock for usb3 controller.
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: usb3_lpm_clk
>>> +      - const: usb3_bus_clk
>>> +      - const: usb3_aclk
>>> +      - const: usb3_ipg_clk
>>> +      - const: usb3_core_pclk
>>> +
>>> +  assigned-clocks:
>>> +    items:
>>> +      - description: Phandle and clock specifier of IMX_SC_PM_CLK_PER.
>>> +      - description: Phandle and clock specifoer of IMX_SC_PM_CLK_MISC.
>>> +      - description: Phandle and clock specifoer of
>> IMX_SC_PM_CLK_MST_BUS.
>>> +
>>> +  assigned-clock-rates:
>>> +    items:
>>> +      - description: Must be 125 Mhz.
>>> +      - description: Must be 12 Mhz.
>>> +      - description: Must be 250 Mhz.
>>
>> I would argue that both properties above are not needed. If your
>> hardware requires fixed frequencies, clock provider can fix them, can't it?
> 
> Clock provider don't know fixed value and turn on only used by client.

So maybe fix the clock provider? Or this device driver? Requiring by
binding specific frequencies for every board is a bit redundant.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-19 11:13       ` Krzysztof Kozlowski
@ 2023-03-20 14:49         ` Frank Li
  2023-03-20 15:23           ` Krzysztof Kozlowski
  0 siblings, 1 reply; 17+ messages in thread
From: Frank Li @ 2023-03-20 14:49 UTC (permalink / raw)
  To: Krzysztof Kozlowski, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer



> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Sunday, March 19, 2023 6:13 AM
> To: Frank Li <frank.li@nxp.com>; shawnguo@kernel.org
> Cc: devicetree@vger.kernel.org; festevam@gmail.com; imx@lists.linux.dev;
> kernel@pengutronix.de; krzysztof.kozlowski+dt@linaro.org; linux-arm-
> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; linux-
> kernel@vger.kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
> Subject: Re: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add
> imx8qm cdns3 glue bindings
> 
> Caution: EXT Email
> 
> On 17/03/2023 15:55, Frank Li wrote:
> >
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Friday, March 17, 2023 4:09 AM
> >> To: Frank Li <frank.li@nxp.com>; shawnguo@kernel.org
> >> Cc: devicetree@vger.kernel.org; festevam@gmail.com;
> imx@lists.linux.dev;
> >> kernel@pengutronix.de; krzysztof.kozlowski+dt@linaro.org; linux-arm-
> >> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; linux-
> >> kernel@vger.kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
> >> Subject: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add
> >> imx8qm cdns3 glue bindings
> >>
> >> Caution: EXT Email
> >>
> >> On 16/03/2023 22:27, Frank Li wrote:
> >>> NXP imx8qm integrates 1 cdns3 IP. This is glue layer device bindings.
> >>>
> >>
> >> Subject: drop second/last, redundant "bindings". The "dt-bindings"
> >> prefix is already stating that these are bindings.
> >>
> >>> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> >>> ---
> >>>  .../bindings/usb/fsl,imx8qm-cdns3.yaml        | 122
> ++++++++++++++++++
> >>>  1 file changed, 122 insertions(+)
> >>>  create mode 100644
> Documentation/devicetree/bindings/usb/fsl,imx8qm-
> >> cdns3.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/fsl,imx8qm-
> >> cdns3.yaml b/Documentation/devicetree/bindings/usb/fsl,imx8qm-
> >> cdns3.yaml
> >>> new file mode 100644
> >>> index 000000000000..fc24df1e4483
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/usb/fsl,imx8qm-cdns3.yaml
> >>> @@ -0,0 +1,122 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +# Copyright (c) 2020 NXP
> >>> +%YAML 1.2
> >>> +---
> >>> +$id:
> >>
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
> %2F&data=05%7C01%7Cfrank.li%40nxp.com%7Cc2d76d3694194fba130a08db
> 286ae90e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6381482118
> 66106607%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Lo
> JUcnJnBaGjywN1zF%2BuUpFVUmldixTci96NPzVuio0%3D&reserved=0
> >> tree.org%2Fschemas%2Fusb%2Ffsl%2Cimx8qm-
> >>
> cdns3.yaml%23&data=05%7C01%7CFrank.Li%40nxp.com%7Cac9af3d617dc4cf
> >>
> 14baf08db26c74b07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6
> >>
> 38146409617970248%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMD
> >>
> AiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C
> >>
> &sdata=EczZhjyMUGnnp7ZGfSvTj4lmOUuGlOtWYIsxxXIlNXw%3D&reserved
> >> =0
> >>> +$schema:
> >>
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevice
> %2F&data=05%7C01%7Cfrank.li%40nxp.com%7Cc2d76d3694194fba130a08db
> 286ae90e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6381482118
> 66106607%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=Lo
> JUcnJnBaGjywN1zF%2BuUpFVUmldixTci96NPzVuio0%3D&reserved=0
> >> tree.org%2Fmeta-
> >>
> schemas%2Fcore.yaml%23&data=05%7C01%7CFrank.Li%40nxp.com%7Cac9a
> >>
> f3d617dc4cf14baf08db26c74b07%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
> >>
> 0%7C0%7C638146409617970248%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiM
> >>
> C4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000
> >> %7C%7C%7C&sdata=uTNYuDm%2ByhZ56oQET2pX8sHpEqVvsUYtmOBCPX
> EK
> >> v40%3D&reserved=0
> >>> +
> >>> +title: NXP iMX8QM Soc USB Controller
> >>> +
> >>> +maintainers:
> >>> +  - Frank Li <Frank.Li@nxp.com>
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    const: fsl,imx8qm-usb3
> >>> +
> >>> +  reg:
> >>> +    items:
> >>> +      - description: Address and length of the register set for iMX USB3
> >> Platform Control
> >>
> >> Drop "Address and length of the"... or actually just maxItems: 1,
> >> because the description is a bit obvious.
> >>
> >>> +
> >>> +  "#address-cells":
> >>> +    enum: [ 1, 2 ]
> >>> +
> >>> +  "#size-cells":
> >>> +    enum: [ 1, 2 ]
> >>> +
> >>> +  ranges: true
> >>> +
> >>> +  clocks:
> >>> +    description:
> >>> +      A list of phandle and clock-specifier pairs for the clocks
> >>> +      listed in clock-names.
> >>
> >> Drop description.
> >>
> >>> +    items:
> >>> +      - description: Standby clock. Used during ultra low power states.
> >>> +      - description: USB bus clock for usb3 controller.
> >>> +      - description: AXI clock for AXI interface.
> >>> +      - description: ipg clock for register access.
> >>> +      - description: Core clock for usb3 controller.
> >>> +
> >>> +  clock-names:
> >>> +    items:
> >>> +      - const: usb3_lpm_clk
> >>> +      - const: usb3_bus_clk
> >>> +      - const: usb3_aclk
> >>> +      - const: usb3_ipg_clk
> >>> +      - const: usb3_core_pclk
> >>> +
> >>> +  assigned-clocks:
> >>> +    items:
> >>> +      - description: Phandle and clock specifier of IMX_SC_PM_CLK_PER.
> >>> +      - description: Phandle and clock specifoer of
> IMX_SC_PM_CLK_MISC.
> >>> +      - description: Phandle and clock specifoer of
> >> IMX_SC_PM_CLK_MST_BUS.
> >>> +
> >>> +  assigned-clock-rates:
> >>> +    items:
> >>> +      - description: Must be 125 Mhz.
> >>> +      - description: Must be 12 Mhz.
> >>> +      - description: Must be 250 Mhz.
> >>
> >> I would argue that both properties above are not needed. If your
> >> hardware requires fixed frequencies, clock provider can fix them, can't it?
> >
> > Clock provider don't know fixed value and turn on only used by client.
> 
> So maybe fix the clock provider? Or this device driver? Requiring by
> binding specific frequencies for every board is a bit redundant.

It is not for every boards, it is common for a chip family.  Only a place to set for
QM and QXP. 

The similar case is network driver, which require a specific frequency at clock assign.
Generally frequency is fixed,  clock source name may change at difference chips. 

> 
> Best regards,
> Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-20 14:49         ` Frank Li
@ 2023-03-20 15:23           ` Krzysztof Kozlowski
  2023-03-20 16:22             ` Frank Li
  0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-20 15:23 UTC (permalink / raw)
  To: Frank Li, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer

On 20/03/2023 15:49, Frank Li wrote:
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: usb3_lpm_clk
>>>>> +      - const: usb3_bus_clk
>>>>> +      - const: usb3_aclk
>>>>> +      - const: usb3_ipg_clk
>>>>> +      - const: usb3_core_pclk
>>>>> +
>>>>> +  assigned-clocks:
>>>>> +    items:
>>>>> +      - description: Phandle and clock specifier of IMX_SC_PM_CLK_PER.
>>>>> +      - description: Phandle and clock specifoer of
>> IMX_SC_PM_CLK_MISC.
>>>>> +      - description: Phandle and clock specifoer of
>>>> IMX_SC_PM_CLK_MST_BUS.
>>>>> +
>>>>> +  assigned-clock-rates:
>>>>> +    items:
>>>>> +      - description: Must be 125 Mhz.
>>>>> +      - description: Must be 12 Mhz.
>>>>> +      - description: Must be 250 Mhz.
>>>>
>>>> I would argue that both properties above are not needed. If your
>>>> hardware requires fixed frequencies, clock provider can fix them, can't it?
>>>
>>> Clock provider don't know fixed value and turn on only used by client.
>>
>> So maybe fix the clock provider? Or this device driver? Requiring by
>> binding specific frequencies for every board is a bit redundant.
> 
> It is not for every boards, it is common for a chip family.  Only a place to set for
> QM and QXP. 
> 
> The similar case is network driver, which require a specific frequency at clock assign.
> Generally frequency is fixed,  clock source name may change at difference chips. 

If frequency is always fixed, I don't understand why this is in DT
bindings. I would even say it should not be in DTS. We don't put into
DTS properties which are always the same, because otherwise they would
grow crazy big.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-20 15:23           ` Krzysztof Kozlowski
@ 2023-03-20 16:22             ` Frank Li
  2023-03-20 16:27               ` Krzysztof Kozlowski
  0 siblings, 1 reply; 17+ messages in thread
From: Frank Li @ 2023-03-20 16:22 UTC (permalink / raw)
  To: Krzysztof Kozlowski, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer

> >>>>> +  assigned-clocks:
> >>>>> +    items:
> >>>>> +      - description: Phandle and clock specifier of
> IMX_SC_PM_CLK_PER.
> >>>>> +      - description: Phandle and clock specifoer of
> >> IMX_SC_PM_CLK_MISC.
> >>>>> +      - description: Phandle and clock specifoer of
> >>>> IMX_SC_PM_CLK_MST_BUS.
> >>>>> +
> >>>>> +  assigned-clock-rates:
> >>>>> +    items:
> >>>>> +      - description: Must be 125 Mhz.
> >>>>> +      - description: Must be 12 Mhz.
> >>>>> +      - description: Must be 250 Mhz.
> >>>>
> >>>> I would argue that both properties above are not needed. If your
> >>>> hardware requires fixed frequencies, clock provider can fix them, can't
> it?
> >>>
> >>> Clock provider don't know fixed value and turn on only used by client.
> >>
> >> So maybe fix the clock provider? Or this device driver? Requiring by
> >> binding specific frequencies for every board is a bit redundant.
> >
> > It is not for every boards, it is common for a chip family.  Only a place to set
> for
> > QM and QXP.
> >
> > The similar case is network driver, which require a specific frequency at
> clock assign.
> > Generally frequency is fixed,  clock source name may change at difference
> chips.
> 
> If frequency is always fixed, I don't understand why this is in DT
> bindings. I would even say it should not be in DTS. We don't put into
> DTS properties which are always the same, because otherwise they would
> grow crazy big.

Although frequency is fixed, clock name may change for difference platform.

                assigned-clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
                                 	     <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
                                 	     <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MST_BUS>;
               assigned-clock-rates = <125000000>, <12000000>, <250000000>;
 
some platform use IMX_SC_R_USB_2, other platform may use IMX_SC_R_USB_3.


> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-20 16:22             ` Frank Li
@ 2023-03-20 16:27               ` Krzysztof Kozlowski
  2023-03-20 17:02                 ` Frank Li
  0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-20 16:27 UTC (permalink / raw)
  To: Frank Li, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer

On 20/03/2023 17:22, Frank Li wrote:
>>>>>>> +  assigned-clocks:
>>>>>>> +    items:
>>>>>>> +      - description: Phandle and clock specifier of
>> IMX_SC_PM_CLK_PER.
>>>>>>> +      - description: Phandle and clock specifoer of
>>>> IMX_SC_PM_CLK_MISC.
>>>>>>> +      - description: Phandle and clock specifoer of
>>>>>> IMX_SC_PM_CLK_MST_BUS.
>>>>>>> +
>>>>>>> +  assigned-clock-rates:
>>>>>>> +    items:
>>>>>>> +      - description: Must be 125 Mhz.
>>>>>>> +      - description: Must be 12 Mhz.
>>>>>>> +      - description: Must be 250 Mhz.
>>>>>>
>>>>>> I would argue that both properties above are not needed. If your
>>>>>> hardware requires fixed frequencies, clock provider can fix them, can't
>> it?
>>>>>
>>>>> Clock provider don't know fixed value and turn on only used by client.
>>>>
>>>> So maybe fix the clock provider? Or this device driver? Requiring by
>>>> binding specific frequencies for every board is a bit redundant.
>>>
>>> It is not for every boards, it is common for a chip family.  Only a place to set
>> for
>>> QM and QXP.
>>>
>>> The similar case is network driver, which require a specific frequency at
>> clock assign.
>>> Generally frequency is fixed,  clock source name may change at difference
>> chips.
>>
>> If frequency is always fixed, I don't understand why this is in DT
>> bindings. I would even say it should not be in DTS. We don't put into
>> DTS properties which are always the same, because otherwise they would
>> grow crazy big.
> 
> Although frequency is fixed, clock name may change for difference platform.
> 
>                 assigned-clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
>                                  	     <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
>                                  	     <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MST_BUS>;
>                assigned-clock-rates = <125000000>, <12000000>, <250000000>;
>  
> some platform use IMX_SC_R_USB_2, other platform may use IMX_SC_R_USB_3.

This I understand, you wrote it above, so nothing new and my concerns
are still there.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-20 16:27               ` Krzysztof Kozlowski
@ 2023-03-20 17:02                 ` Frank Li
  2023-03-20 17:28                   ` Krzysztof Kozlowski
  0 siblings, 1 reply; 17+ messages in thread
From: Frank Li @ 2023-03-20 17:02 UTC (permalink / raw)
  To: Krzysztof Kozlowski, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer



> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Sent: Monday, March 20, 2023 11:27 AM
> To: Frank Li <frank.li@nxp.com>; shawnguo@kernel.org
> Cc: devicetree@vger.kernel.org; festevam@gmail.com; imx@lists.linux.dev;
> kernel@pengutronix.de; krzysztof.kozlowski+dt@linaro.org; linux-arm-
> kernel@lists.infradead.org; dl-linux-imx <linux-imx@nxp.com>; linux-
> kernel@vger.kernel.org; robh+dt@kernel.org; s.hauer@pengutronix.de
> Subject: Re: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add
> imx8qm cdns3 glue bindings
> 
> Caution: EXT Email
> 
> On 20/03/2023 17:22, Frank Li wrote:
> >>>>>>> +  assigned-clocks:
> >>>>>>> +    items:
> >>>>>>> +      - description: Phandle and clock specifier of
> >> IMX_SC_PM_CLK_PER.
> >>>>>>> +      - description: Phandle and clock specifoer of
> >>>> IMX_SC_PM_CLK_MISC.
> >>>>>>> +      - description: Phandle and clock specifoer of
> >>>>>> IMX_SC_PM_CLK_MST_BUS.
> >>>>>>> +
> >>>>>>> +  assigned-clock-rates:
> >>>>>>> +    items:
> >>>>>>> +      - description: Must be 125 Mhz.
> >>>>>>> +      - description: Must be 12 Mhz.
> >>>>>>> +      - description: Must be 250 Mhz.
> >>>>>>
> >>>>>> I would argue that both properties above are not needed. If your
> >>>>>> hardware requires fixed frequencies, clock provider can fix them,
> can't
> >> it?
> >>>>>
> >>>>> Clock provider don't know fixed value and turn on only used by client.
> >>>>
> >>>> So maybe fix the clock provider? Or this device driver? Requiring by
> >>>> binding specific frequencies for every board is a bit redundant.
> >>>
> >>> It is not for every boards, it is common for a chip family.  Only a place to
> set
> >> for
> >>> QM and QXP.
> >>>
> >>> The similar case is network driver, which require a specific frequency at
> >> clock assign.
> >>> Generally frequency is fixed,  clock source name may change at
> difference
> >> chips.
> >>
> >> If frequency is always fixed, I don't understand why this is in DT
> >> bindings. I would even say it should not be in DTS. We don't put into
> >> DTS properties which are always the same, because otherwise they would
> >> grow crazy big.
> >
> > Although frequency is fixed, clock name may change for difference
> platform.
> >
> >                 assigned-clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
> >                                            <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
> >                                            <&clk IMX_SC_R_USB_2
> IMX_SC_PM_CLK_MST_BUS>;
> >                assigned-clock-rates = <125000000>, <12000000>, <250000000>;
> >
> > some platform use IMX_SC_R_USB_2, other platform may use
> IMX_SC_R_USB_3.
> 
> This I understand, you wrote it above, so nothing new and my concerns
> are still there.

I think Fixed value is not good reason. All reg base address, irq number are all for fixed number. The same
Logic can be applied to irq-provider driver. But why still be descript in dts? It is hardware property.    

https://elixir.bootlin.com/linux/v4.8/source/Documentation/devicetree/bindings/clock/clock-bindings.txt
have not said that can't set to fixed clock frequency. 

This is quick common case for network, USB, SATA, PCIE,  which protocol defined
Frequency.  

https://elixir.bootlin.com/linux/v6.3-rc3/source/Documentation/devicetree/bindings/ata/qcom-sata.txt
https://elixir.bootlin.com/linux/v6.3-rc3/source/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml

Such frequency information is necessary.  We can put to dts or clock drivers.  The clock driver
Become bigger, or dts become bigger.  I think the key point is if property to descript hardware information.  

> 
> Best regards,
> Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-20 17:02                 ` Frank Li
@ 2023-03-20 17:28                   ` Krzysztof Kozlowski
  2023-03-20 19:59                     ` Frank Li
  0 siblings, 1 reply; 17+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-20 17:28 UTC (permalink / raw)
  To: Frank Li, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer

On 20/03/2023 18:02, Frank Li wrote:

>>> Although frequency is fixed, clock name may change for difference
>> platform.
>>>
>>>                 assigned-clocks = <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_PER>,
>>>                                            <&clk IMX_SC_R_USB_2 IMX_SC_PM_CLK_MISC>,
>>>                                            <&clk IMX_SC_R_USB_2
>> IMX_SC_PM_CLK_MST_BUS>;
>>>                assigned-clock-rates = <125000000>, <12000000>, <250000000>;
>>>
>>> some platform use IMX_SC_R_USB_2, other platform may use
>> IMX_SC_R_USB_3.
>>
>> This I understand, you wrote it above, so nothing new and my concerns
>> are still there.
> 
> I think Fixed value is not good reason. All reg base address, irq number are all for fixed number. The same

No, because one device - IP block - could have different addresses,
depending how it is wired/implemented in given SoC. Also our
representation of devices in the kernel requires regs/interrupts coming
from DTS, thus DTS is also answer to entire design of kernel and other SW.

That's not the case here at all.

> Logic can be applied to irq-provider driver. But why still be descript in dts? It is hardware property.    
> 
> https://elixir.bootlin.com/linux/v4.8/source/Documentation/devicetree/bindings/clock/clock-bindings.txt
> have not said that can't set to fixed clock frequency. 

I don't understand this.

> 
> This is quick common case for network, USB, SATA, PCIE,  which protocol defined
> Frequency.  

And they do not define fixed values in the bindings, so? There are
exceptions, but it's usually not argument, right?

> 
> https://elixir.bootlin.com/linux/v6.3-rc3/source/Documentation/devicetree/bindings/ata/qcom-sata.txt
> https://elixir.bootlin.com/linux/v6.3-rc3/source/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml

The second is a good example - as you can see, there is a choice of
values, so they are not exactly fixed.

> 
> Such frequency information is necessary.  We can put to dts or clock drivers.  The clock driver

If this is the argument, then the answer is NAK. Sorry, but DTS is not
for offloading fixed stuff just because you do not want to work on
drivers. The same for discoverable stuff.

> Become bigger, or dts become bigger.  I think the key point is if property to descript hardware information.  

You have to understand that with your binding you are not allowing to
any changes of these frequencies.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* RE: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-20 17:28                   ` Krzysztof Kozlowski
@ 2023-03-20 19:59                     ` Frank Li
  2023-03-21  6:37                       ` Krzysztof Kozlowski
  0 siblings, 1 reply; 17+ messages in thread
From: Frank Li @ 2023-03-20 19:59 UTC (permalink / raw)
  To: Krzysztof Kozlowski, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer

> 
> The second is a good example - as you can see, there is a choice of
> values, so they are not exactly fixed.
> 
> >
> > Such frequency information is necessary.  We can put to dts or clock drivers.
> The clock driver
> 
> If this is the argument, then the answer is NAK. Sorry, but DTS is not
> for offloading fixed stuff just because you do not want to work on
> drivers. The same for discoverable stuff.
> 
> > Become bigger, or dts become bigger.  I think the key point is if property to
> descript hardware information.
> 
> You have to understand that with your binding you are not allowing to
> any changes of these frequencies.

Do you means it should be okay if one of  clocks is not fixed?

Previous owner already left nxp.  I double checked our documents and scfw source code. 
I miss understood a clock SC_PM_CLK_MST_BUS, which actually mapped to IP's aclk, which
ware 100Mhz to 600Mhz. 

> 
> Best regards,
> Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings
  2023-03-20 19:59                     ` Frank Li
@ 2023-03-21  6:37                       ` Krzysztof Kozlowski
  0 siblings, 0 replies; 17+ messages in thread
From: Krzysztof Kozlowski @ 2023-03-21  6:37 UTC (permalink / raw)
  To: Frank Li, shawnguo
  Cc: devicetree, festevam, imx, kernel, krzysztof.kozlowski+dt,
	linux-arm-kernel, dl-linux-imx, linux-kernel, robh+dt, s.hauer

On 20/03/2023 20:59, Frank Li wrote:
>>
>> The second is a good example - as you can see, there is a choice of
>> values, so they are not exactly fixed.
>>
>>>
>>> Such frequency information is necessary.  We can put to dts or clock drivers.
>> The clock driver
>>
>> If this is the argument, then the answer is NAK. Sorry, but DTS is not
>> for offloading fixed stuff just because you do not want to work on
>> drivers. The same for discoverable stuff.
>>
>>> Become bigger, or dts become bigger.  I think the key point is if property to
>> descript hardware information.
>>
>> You have to understand that with your binding you are not allowing to
>> any changes of these frequencies.
> 
> Do you means it should be okay if one of  clocks is not fixed?

We have here long discussion why does your binding require fixed
frequencies, because this is something unusual and not recommended. And
if they are really fixed, then device driver probably should make the
choice of frequencies.

> 
> Previous owner already left nxp.  I double checked our documents and scfw source code. 
> I miss understood a clock SC_PM_CLK_MST_BUS, which actually mapped to IP's aclk, which
> ware 100Mhz to 600Mhz. 
> 
>>
>> Best regards,
>> Krzysztof
> 

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2023-03-21  6:38 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-16 21:27 [PATCH v2 0/3] dts: imx8qxp add cdns usb3 port Frank Li
2023-03-16 21:27 ` [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings Frank Li
2023-03-17  9:09   ` Krzysztof Kozlowski
2023-03-17 14:55     ` [EXT] " Frank Li
2023-03-19 11:13       ` Krzysztof Kozlowski
2023-03-20 14:49         ` Frank Li
2023-03-20 15:23           ` Krzysztof Kozlowski
2023-03-20 16:22             ` Frank Li
2023-03-20 16:27               ` Krzysztof Kozlowski
2023-03-20 17:02                 ` Frank Li
2023-03-20 17:28                   ` Krzysztof Kozlowski
2023-03-20 19:59                     ` Frank Li
2023-03-21  6:37                       ` Krzysztof Kozlowski
2023-03-16 21:27 ` [PATCH v2 2/3] arm64: dts: imx8qxp: add cadence usb3 support Frank Li
2023-03-17  6:39   ` Alexander Stein
2023-03-17 15:05     ` [EXT] " Frank Li
2023-03-16 21:27 ` [PATCH v2 3/3] arm64: dts: freescale: imx8qxp-mek: enable cadence usb3 Frank Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).