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
NXP imx8qm integrates 1 cdns3 IP. This is glue layer device bindings.
Signed-off-by: Frank Li <[email protected]>
---
.../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 <[email protected]>
+
+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
There are cadence usb3.0 controller in 8qxp and 8qm.
Add usb3 node at common connect subsystem.
Signed-off-by: Frank Li <[email protected]>
---
.../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
Enable USB3 controller, phy and typec related nodes.
Signed-off-by: Frank Li <[email protected]>
---
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
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 <[email protected]>
> ---
> .../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/
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 <[email protected]>
> ---
> .../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 <[email protected]>
> +
> +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
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Friday, March 17, 2023 4:09 AM
> To: Frank Li <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; dl-linux-imx <[email protected]>; linux-
> [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> > ---
> > .../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 <[email protected]>
> > +
> > +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
>
> 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 <[email protected]>
> > ---
> > .../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>,
>
On 17/03/2023 15:55, Frank Li wrote:
>
>
>> -----Original Message-----
>> From: Krzysztof Kozlowski <[email protected]>
>> Sent: Friday, March 17, 2023 4:09 AM
>> To: Frank Li <[email protected]>; [email protected]
>> Cc: [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; linux-arm-
>> [email protected]; dl-linux-imx <[email protected]>; linux-
>> [email protected]; [email protected]; [email protected]
>> 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 <[email protected]>
>>> ---
>>> .../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 <[email protected]>
>>> +
>>> +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
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Sunday, March 19, 2023 6:13 AM
> To: Frank Li <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; dl-linux-imx <[email protected]>; linux-
> [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> >> Sent: Friday, March 17, 2023 4:09 AM
> >> To: Frank Li <[email protected]>; [email protected]
> >> Cc: [email protected]; [email protected];
> [email protected];
> >> [email protected]; [email protected]; linux-arm-
> >> [email protected]; dl-linux-imx <[email protected]>; linux-
> >> [email protected]; [email protected]; [email protected]
> >> 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 <[email protected]>
> >>> ---
> >>> .../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 <[email protected]>
> >>> +
> >>> +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
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
> >>>>> + 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
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
> -----Original Message-----
> From: Krzysztof Kozlowski <[email protected]>
> Sent: Monday, March 20, 2023 11:27 AM
> To: Frank Li <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-arm-
> [email protected]; dl-linux-imx <[email protected]>; linux-
> [email protected]; [email protected]; [email protected]
> 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
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
>
> 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
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