| Message ID | 20240808-qcom_ipq_cmnpll-v1-0-b0631dcbf785@quicinc.com |
|---|---|
| Headers | show |
| Series | Add common PLL clock controller driver for IPQ9574 | expand |
On 08/08/2024 16:03, Luo Jie wrote: > The common PLL controller provides clocks to networking hardware > blocks on Qualcomm IPQ SoC. It receives input clock from the on-chip > Wi-Fi, and produces output clocks at fixed rates. These output rates > are predetermined, and are unrelated to the input clock rate. The > output clocks are supplied to the Ethernet hardware such as PPE > (packet process engine) and the externally connected switch or PHY > device. > > The common PLL driver is initially being supported for IPQ9574 SoC. Drop references to driver and explain the hardware. Above with the usage of "common" looks like this is all for some common driver, not for particular hardware. > > Signed-off-by: Luo Jie <quic_luoj@quicinc.com> > --- > .../bindings/clock/qcom,ipq-cmn-pll.yaml | 87 ++++++++++++++++++++++ > 1 file changed, 87 insertions(+) > > diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml > new file mode 100644 > index 000000000000..c45b3a201751 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml Use compatible as filename. > @@ -0,0 +1,87 @@ > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/clock/qcom,ipq-cmn-pll.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Qualcomm Common PLL Clock Controller on IPQ SoC > + > +maintainers: > + - Bjorn Andersson <andersson@kernel.org> > + - Luo Jie <quic_luoj@quicinc.com> > + > +description: > + The common PLL clock controller expects a reference input clock. > + This reference clock is from the on-board Wi-Fi. The CMN PLL > + supplies a number of fixed rate output clocks to the Ethernet > + devices including PPE (packet process engine) and the connected > + switch or PHY device. > + > +properties: > + compatible: > + enum: > + - qcom,ipq9574-cmn-pll > + > + reg: > + maxItems: 1 > + > + clocks: > + items: > + - description: The reference clock, the supported clock rates include > + 25000000, 31250000, 40000000, 48000000, 50000000 and 96000000 HZ. > + - description: The AHB clock > + - description: The SYS clock > + description: > + The reference clock is the source clock of CMN PLL, which is from the > + Wi-Fi. The AHB and SYS clocks must be enabled to access common PLL > + clock registers. > + > + clock-names: > + items: > + - const: ref > + - const: ahb > + - const: sys > + > + clock-output-names: > + items: > + - const: ppe-353mhz > + - const: eth0-50mhz > + - const: eth1-50mhz > + - const: eth2-50mhz > + - const: eth-25mhz Drop entire property. If the names are fixed, what's the point of having it in DTS? There is no. Best regards, Krzysztof
On 8/8/2024 10:41 PM, Krzysztof Kozlowski wrote: > On 08/08/2024 16:03, Luo Jie wrote: >> The common PLL clock controller provides fixed rate output clocks to >> the hardware blocks that enable ethernet function on IPQ platform. > > That's defconfig for all platforms, so how anyone can guess which one > you target here? Be specific, which company, which Soc, which board > needs it. > Sure, I will update the commit message as below to provide the details required. The common PLL hardware block is available in the Qualcomm IPQ SoC such as IPQ9574 and IPQ5332. It provides fixed rate output clocks to Ethernet related hardware blocks such as external Ethernet PHY or switch. This driver is initially being enabled for IPQ9574. All boards based on IPQ9574 SoC will require to include this driver in the build. > > > Best regards, > Krzysztof >
On 8/8/2024 10:38 PM, Krzysztof Kozlowski wrote: > On 08/08/2024 16:03, Luo Jie wrote: >> The common PLL controller provides clocks to networking hardware >> blocks on Qualcomm IPQ SoC. It receives input clock from the on-chip >> Wi-Fi, and produces output clocks at fixed rates. These output rates >> are predetermined, and are unrelated to the input clock rate. The >> output clocks are supplied to the Ethernet hardware such as PPE >> (packet process engine) and the externally connected switch or PHY >> device. >> >> The common PLL driver is initially being supported for IPQ9574 SoC. > > Drop references to driver and explain the hardware. > > Above with the usage of "common" looks like this is all for some common > driver, not for particular hardware. Understand, will remove this driver reference. > >> >> Signed-off-by: Luo Jie <quic_luoj@quicinc.com> >> --- >> .../bindings/clock/qcom,ipq-cmn-pll.yaml | 87 ++++++++++++++++++++++ >> 1 file changed, 87 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml >> new file mode 100644 >> index 000000000000..c45b3a201751 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/qcom,ipq-cmn-pll.yaml > > Use compatible as filename. OK. > >> @@ -0,0 +1,87 @@ >> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/clock/qcom,ipq-cmn-pll.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Qualcomm Common PLL Clock Controller on IPQ SoC >> + >> +maintainers: >> + - Bjorn Andersson <andersson@kernel.org> >> + - Luo Jie <quic_luoj@quicinc.com> >> + >> +description: >> + The common PLL clock controller expects a reference input clock. >> + This reference clock is from the on-board Wi-Fi. The CMN PLL >> + supplies a number of fixed rate output clocks to the Ethernet >> + devices including PPE (packet process engine) and the connected >> + switch or PHY device. >> + >> +properties: >> + compatible: >> + enum: >> + - qcom,ipq9574-cmn-pll >> + >> + reg: >> + maxItems: 1 >> + >> + clocks: >> + items: >> + - description: The reference clock, the supported clock rates include >> + 25000000, 31250000, 40000000, 48000000, 50000000 and 96000000 HZ. >> + - description: The AHB clock >> + - description: The SYS clock >> + description: >> + The reference clock is the source clock of CMN PLL, which is from the >> + Wi-Fi. The AHB and SYS clocks must be enabled to access common PLL >> + clock registers. >> + >> + clock-names: >> + items: >> + - const: ref >> + - const: ahb >> + - const: sys >> + >> + clock-output-names: >> + items: >> + - const: ppe-353mhz >> + - const: eth0-50mhz >> + - const: eth1-50mhz >> + - const: eth2-50mhz >> + - const: eth-25mhz > > Drop entire property. If the names are fixed, what's the point of having > it in DTS? There is no. We had added the output names here for the reasons below. Can you please let us know your suggestion whether keeping these here is fine? 1.) These output clocks are used as input reference clocks to other consumer blocks. For example, an on-board Ethernet PHY device may be wired to receive a specific clock from the above output clocks as reference clock input, and hence the PHY's DTS node would need to reference a particular index in this output clock array. Without these output clocks being made available in this DTS, the PHY driver in above case would not know the clock specifier to access the handle for the desired input clock. 2.) One of the suggestions from the internal code review with Linaro was to name the output clocks specifically based on rate and destination (Ex: 'ppe-353mhz' for fixed rate 353 MHZ output clock connected to Packet Process Engine block), so that the dt-bindings describe the input/output clocks clearly. > > Best regards, > Krzysztof >
On Fri, Aug 09, 2024 at 07:36:35PM +0800, Jie Luo wrote: > > > On 8/8/2024 10:41 PM, Krzysztof Kozlowski wrote: > > On 08/08/2024 16:03, Luo Jie wrote: > > > The common PLL clock controller provides fixed rate output clocks to > > > the hardware blocks that enable ethernet function on IPQ platform. > > > > That's defconfig for all platforms, so how anyone can guess which one > > you target here? Be specific, which company, which Soc, which board > > needs it. > > > > Sure, I will update the commit message as below to provide the details > required. > > The common PLL hardware block is available in the Qualcomm IPQ SoC such > as IPQ9574 and IPQ5332. It provides fixed rate output clocks to Ethernet > related hardware blocks such as external Ethernet PHY or switch. This > driver is initially being enabled for IPQ9574. All boards based on > IPQ9574 SoC will require to include this driver in the build. Does it provide more than Ethernet clocks? I'm just wondering why the name `common`, when it seems pretty uncommon, specialised for Ethernet clocks on a couple of SoCs. Andrew
On 09/08/2024 15:01, Jie Luo wrote: >>> + clock-names: >>> + items: >>> + - const: ref >>> + - const: ahb >>> + - const: sys >>> + >>> + clock-output-names: >>> + items: >>> + - const: ppe-353mhz >>> + - const: eth0-50mhz >>> + - const: eth1-50mhz >>> + - const: eth2-50mhz >>> + - const: eth-25mhz >> >> Drop entire property. If the names are fixed, what's the point of having >> it in DTS? There is no. > > We had added the output names here for the reasons below. Can you please > let us know your suggestion whether keeping these here is fine? > > 1.) These output clocks are used as input reference clocks to other > consumer blocks. For example, an on-board Ethernet PHY device may be > wired to receive a specific clock from the above output clocks as > reference clock input, and hence the PHY's DTS node would need to > reference a particular index in this output clock array. > > Without these output clocks being made available in this DTS, the PHY > driver in above case would not know the clock specifier to access the > handle for the desired input clock. That's not true. clock-output-names do not have anything to do with clock specifier. > > 2.) One of the suggestions from the internal code review with Linaro was > to name the output clocks specifically based on rate and destination > (Ex: 'ppe-353mhz' for fixed rate 353 MHZ output clock connected to > Packet Process Engine block), so that the dt-bindings describe the > input/output clocks clearly. Again, that's unrelated. None of above points address my concern. It's like you talk about some entirely different topic. Again: clock-output-names have nothing to do with what you want to achieve here. Best regards, Krzysztof
On 8/9/2024 9:34 PM, Andrew Lunn wrote: > On Fri, Aug 09, 2024 at 07:36:35PM +0800, Jie Luo wrote: >> >> >> On 8/8/2024 10:41 PM, Krzysztof Kozlowski wrote: >>> On 08/08/2024 16:03, Luo Jie wrote: >>>> The common PLL clock controller provides fixed rate output clocks to >>>> the hardware blocks that enable ethernet function on IPQ platform. >>> >>> That's defconfig for all platforms, so how anyone can guess which one >>> you target here? Be specific, which company, which Soc, which board >>> needs it. >>> >> >> Sure, I will update the commit message as below to provide the details >> required. >> >> The common PLL hardware block is available in the Qualcomm IPQ SoC such >> as IPQ9574 and IPQ5332. It provides fixed rate output clocks to Ethernet >> related hardware blocks such as external Ethernet PHY or switch. This >> driver is initially being enabled for IPQ9574. All boards based on >> IPQ9574 SoC will require to include this driver in the build. > > Does it provide more than Ethernet clocks? I'm just wondering why the > name `common`, when it seems pretty uncommon, specialised for Ethernet > clocks on a couple of SoCs. > > Andrew No, this block does not provide any other functionality other than allowing this PLL to be configured for supplying clocks to Ethernet devices. The hardware programming guide names this block as the 'CMN' block, so we included the 'cmn' phrase in the driver namespace. However, I will update commit message to clarify that 'cmn' is the block name and it does not provide any other function other than enabling clocks to Ethernet related devices.
On 8/10/2024 7:30 PM, Krzysztof Kozlowski wrote: > On 09/08/2024 15:01, Jie Luo wrote: >>>> + clock-names: >>>> + items: >>>> + - const: ref >>>> + - const: ahb >>>> + - const: sys >>>> + >>>> + clock-output-names: >>>> + items: >>>> + - const: ppe-353mhz >>>> + - const: eth0-50mhz >>>> + - const: eth1-50mhz >>>> + - const: eth2-50mhz >>>> + - const: eth-25mhz >>> >>> Drop entire property. If the names are fixed, what's the point of having >>> it in DTS? There is no. >> >> We had added the output names here for the reasons below. Can you please >> let us know your suggestion whether keeping these here is fine? >> >> 1.) These output clocks are used as input reference clocks to other >> consumer blocks. For example, an on-board Ethernet PHY device may be >> wired to receive a specific clock from the above output clocks as >> reference clock input, and hence the PHY's DTS node would need to >> reference a particular index in this output clock array. >> >> Without these output clocks being made available in this DTS, the PHY >> driver in above case would not know the clock specifier to access the >> handle for the desired input clock. > > That's not true. clock-output-names do not have anything to do with > clock specifier. > >> >> 2.) One of the suggestions from the internal code review with Linaro was >> to name the output clocks specifically based on rate and destination >> (Ex: 'ppe-353mhz' for fixed rate 353 MHZ output clock connected to >> Packet Process Engine block), so that the dt-bindings describe the >> input/output clocks clearly. > > Again, that's unrelated. None of above points address my concern. It's > like you talk about some entirely different topic. Again: > clock-output-names have nothing to do with what you want to achieve here. OK, understand. I will drop this property "clock-output-names" from the bindings and DTS. These names will instead be defined in the driver. For the consumer clock device DTS nodes that need to reference these output clocks, I will export the clock specifiers for these output clocks from a header file. Hope this approach is fine. > > Best regards, > Krzysztof >
The common PLL clock controller in Qualcomm IPQ chipsets provides the clocks to the networking hardware blocks that are internal or external to the SoC. This driver configures the common PLL clock controller to enable the output clocks to such networking hardware blocks. These networking blocks include the internal PPE (Packet Process Engine), external connected Ethernet PHY, or external switch. The controller expects the input reference clock from the internal Wi-Fi block acting as the clock source. The output clocks supplied by the controller are fixed rate clocks. The driver is being enabled to support IPQ9574 SoC initially, and will be extended for other SoCs. Signed-off-by: Luo Jie <quic_luoj@quicinc.com> --- Luo Jie (4): dt-bindings: clock: qcom: Add common PLL clock controller for IPQ SoC clk: qcom: Add common PLL clock controller driver for IPQ SoC arm64: defconfig: Enable Qualcomm IPQ common PLL clock controller arm64: dts: qcom: Add common PLL node for IPQ9574 SoC .../bindings/clock/qcom,ipq-cmn-pll.yaml | 87 ++++++++ arch/arm64/boot/dts/qcom/ipq9574-rdp-common.dtsi | 6 +- arch/arm64/boot/dts/qcom/ipq9574.dtsi | 22 +- arch/arm64/configs/defconfig | 1 + drivers/clk/qcom/Kconfig | 10 + drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/clk-ipq-cmn-pll.c | 233 +++++++++++++++++++++ 7 files changed, 358 insertions(+), 2 deletions(-) --- base-commit: 222a3380f92b8791d4eeedf7cd750513ff428adf change-id: 20240808-qcom_ipq_cmnpll-7c1119b25037 Best regards,