> -----Original Message----- > From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx> > Sent: Monday, March 20, 2023 11:27 AM > To: Frank Li <frank.li@xxxxxxx>; shawnguo@xxxxxxxxxx > Cc: devicetree@xxxxxxxxxxxxxxx; festevam@xxxxxxxxx; imx@xxxxxxxxxxxxxxx; > kernel@xxxxxxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx; linux-arm- > kernel@xxxxxxxxxxxxxxxxxxx; dl-linux-imx <linux-imx@xxxxxxx>; linux- > kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx > 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
