| Message ID | 20230316212712.2426542-2-Frank.Li@nxp.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings | expand |
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
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 <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
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
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"; + }; + };
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