Messages in this thread | | | From | Frank Li <> | Subject | RE: [EXT] Re: [PATCH v2 1/3] dt-bindings: usb: cdns-imx8qm: add imx8qm cdns3 glue bindings | Date | Mon, 20 Mar 2023 17:02:56 +0000 |
| |
> -----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
| |