| Message ID | 1550771836-10014-3-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | clk: imx: scu: add parsing clocks from device tree support | expand |
Quoting Aisheng Dong (2019-02-21 10:03:47) > MX8QM and MX8QXP LPCG Clocks are mostly the same except they may reside > in different subsystems across CPUs and also vary a bit on the availability. > > Same as SCU clock, we want to move the clock definition into device tree > which can fully decouple the dependency of Clock ID definition from device > tree. And no frequent changes required in clock driver any more to handle > the difference. > > We can use the existence of clock nodes in device tree to address the > device and clock availability differences across different SoCs. This sounds similar to what TI folks are doing with their new firmware. It leads to problems where we don't know what in the clk tree needs to be registered, debugfs is not super helpful in that case, and late init only turns off clks that are found during probe (so nothing then?). > > diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > index 965cfa4..a317844 100644 > --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > @@ -11,6 +11,20 @@ enabled by these control bits, it might still not be running based > on the base resource. > > Required properties: > +- compatible: Should be one of: > + "fsl,imx8qxp-lpcg" > + "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-lpcg". > +- reg: Address and length of the register set. > +- #clock-cells: Should be 1. One LPCG supports multiple clocks. > +- clocks: Input parent clocks phandle array for each clock. > +- bit-offset: An integer array indicating the bit offset for each clock. > +- hw-autogate: Boolean array indicating whether supports HW autogate for > + each clock. This looks like one clk per node style of bindings which is a direction we don't want DT bindings to go in. It leads to a bunch of time parsing DT to generate clks and in general doesn't represent the clock controller hardware that is there. Basically, anything with 'bit-offset' in the binding is not going to be acceptable. > +- clock-output-names: Shall be the corresponding names of the outputs. > + NOTE this property must be specified in the same order > + as the clock bit-offset and hw-autogate property. > + > +Legacy binding (DEPRECATED): > - compatible: Should be one of: > "fsl,imx8qxp-lpcg-adma",
Hi Stephen,
Sorry for the long email, I want to describe things more clear to help the review.
First, i want to share some backgrounds on how this patch series comes from,
it mainly consists of three reasons:
1. New architecture to save a lot duplicated codes
We want to write a more generic <soc>-ss-<subsys>.dtsi shared by imx8qxp, imx8qm,
imx8dx, imx8dm according to HW characteristic then we'd better decouple the dependency
of Clock ID definitions from device tree due to the SS and device availability difference among them.
[00/14] arm64: dts: imx8: architecture improvement and adding imx8qm support
https://patchwork.kernel.org/cover/10824537/
2. Power domain requirements
Both SCU and LPCG clock access requires it's associated power domain enabled, CCF
already supports it and device tree seems to be the best place to describe it.
e.g. arch/arm64/boot/dts/exynos/exynos5433.dtsi
cmu_aud: clock-controller@114c0000 {
compatible = "samsung,exynos5433-cmu-aud";
#clock-cells = <1>;
....
power-domains = <&pd_aud>;
};
Furthermore, if the power domain is off (e.g. during system suspend) the clock state
Within this domain will be lost and we have to restored it after power domain is re-enabled.
We'd like to use common driver suspend/resume to handle it.
(In the future, we might support Runtime state save&restore as well in order to shut
down the whole SS if not used, that also need power domain info from DT).
3. Partition support
IMX SCU firmware supports Resource Partition service which allows each device resource
to be partitioned into different ownership groupings that are associated with different
execution environments including multiple operating systems executing on different
cores, TrustZone, and hypervisor.
That means we can't register all the clocks in Linux as some of them may not belong
to us and can't access. (we can check all the clocks via SCU RPC call before register,
but that also needs a branch of time. However, LPCG not easy to check as it does not provide
resource id). Putting clocks of device in DT allows the dynamically configuration of it according
to the real partition requirements.
E.g. Remove some clock/device nodes once not belong to Linux OS partition or not exist in
hardware on this SoC SS.
And please see below for my replies to other of your questions:
> From: Stephen Boyd [mailto:sboyd@kernel.org]
> Sent: Tuesday, February 26, 2019 3:46 AM
> Quoting Aisheng Dong (2019-02-21 10:03:47)
> > MX8QM and MX8QXP LPCG Clocks are mostly the same except they may
> > reside in different subsystems across CPUs and also vary a bit on the
> availability.
> >
> > Same as SCU clock, we want to move the clock definition into device
> > tree which can fully decouple the dependency of Clock ID definition
> > from device tree. And no frequent changes required in clock driver any
> > more to handle the difference.
> >
> > We can use the existence of clock nodes in device tree to address the
> > device and clock availability differences across different SoCs.
>
> This sounds similar to what TI folks are doing with their new firmware.
> It leads to problems where we don't know what in the clk tree needs to be
> registered,
AFAICT we know what clocks need to be registered according to the device availability
in each SS (Subsystem) in HW definition.
Am I missed something?
> debugfs is not super helpful in that case,
Debugfs functions the same as defining clocks in driver.
Every cock defined in driver can be defined in device tree according to HW configuration
and displayed with debugfs. So I'm not quite get the point of the concern.
Can you help clarify a bit more?
> and late init only turns off
> clks that are found during probe (so nothing then?).
Same as above.
BTW, we did not support turn off clocks for LPCG during late init.
Probably won't support in the future as LPCG is the next level gates of SCU clocks.
Gating off LPCG clocks while SCU clocks disabled already looks not so meaningful, right?
And gate off such type LPCG is quite expensive as LPCG needs enable its power domain
to access and chip reset already ensures the LPCG clocks are off and the later LPCG
enable/disable also can sync the HW state.
For the parent SCU clocks, we also still don't have the plan to support late gate off because
SCU clock might be shared with M-Core OS or Secure SW (e.g. ATF, OPTee) and A core can't
Gate off those "unused" ones it believes. Currently what we're doing is ensure gate off
power&clocks after using in bootloader before hand over to kernel.
>
> >
> > diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > index 965cfa4..a317844 100644
> > --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > @@ -11,6 +11,20 @@ enabled by these control bits, it might still not
> > be running based on the base resource.
> >
> > Required properties:
> > +- compatible: Should be one of:
> > + "fsl,imx8qxp-lpcg"
> > + "fsl,imx8qm-lpcg" followed by
> "fsl,imx8qxp-lpcg".
> > +- reg: Address and length of the register set.
> > +- #clock-cells: Should be 1. One LPCG supports multiple
> clocks.
> > +- clocks: Input parent clocks phandle array for each clock.
> > +- bit-offset: An integer array indicating the bit offset for each
> clock.
> > +- hw-autogate: Boolean array indicating whether supports HW
> autogate for
> > + each clock.
>
> This looks like one clk per node style of bindings which is a direction we don't
> want DT bindings to go in. It leads to a bunch of time parsing DT to generate
> clks and in general doesn't represent the clock controller hardware that is
> there. Basically, anything with 'bit-offset'
> in the binding is not going to be acceptable.
>
It's one LPCG per node which represents a couple of clock output gates controlled by
this LPCG for one specific module.
For MX8QM/QXP platforms, there're separate LPCGs for each device resource.
LPCGs are independent with each other with separate io space, behaving separate module
clock controllers and they are distributed in different SS (subsystems).
Describing it separately in device tree is more comply with real hardware although
it sacrifices a bit parsing time.
Regards
Dong Aisheng
> > +- clock-output-names: Shall be the corresponding names of the outputs.
> > + NOTE this property must be specified in the
> same order
> > + as the clock bit-offset and hw-autogate property.
> > +
> > +Legacy binding (DEPRECATED):
> > - compatible: Should be one of:
> > "fsl,imx8qxp-lpcg-adma",
[...] > > diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > > b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > > index 965cfa4..a317844 100644 > > --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > > +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > > @@ -11,6 +11,20 @@ enabled by these control bits, it might still not > > be running based on the base resource. > > > > Required properties: > > +- compatible: Should be one of: > > + "fsl,imx8qxp-lpcg" > > + "fsl,imx8qm-lpcg" followed by > "fsl,imx8qxp-lpcg". > > +- reg: Address and length of the register set. > > +- #clock-cells: Should be 1. One LPCG supports multiple > clocks. > > +- clocks: Input parent clocks phandle array for each clock. > > +- bit-offset: An integer array indicating the bit offset for each > clock. > > +- hw-autogate: Boolean array indicating whether supports HW > autogate for > > + each clock. > > This looks like one clk per node style of bindings which is a direction we don't > want DT bindings to go in. It leads to a bunch of time parsing DT to generate > clks and in general doesn't represent the clock controller hardware that is > there. Basically, anything with 'bit-offset' > in the binding is not going to be acceptable. > This is not one clk per node but one clock controller per node which strictly describes the HW. On MX8, each LPCG is a separate clock controller which can control a couple of clock output gates for one specific device to use. Each device has a corresponding LPCG clock controller and those LPCGs are independent with each other with separate IO space. Those LPCGs are distributed in various SS (subsystems) along with device resources. Describing them in device tree SS dtsi doesn't seem to be an issue as it is representing the real hardware. For SCU clocks, they're similar case that each SS having separate clock controllers In HW which are managed by SCU firmware. So it seems also okay to put them in device tree SS dtsi file, right? If you're concerning each node having one compatible string, how about doing like many power domain does as below? Having only one compatible string in clock parent nodes. //LSIO SS lsio_scu_clk: lsio-scu-clock-controller { compatible = "fsl,imx8qxp-clock", "fsl,scu-clk"; fspi0_clk: clock-fspi0{ #clock-cells = <0>; rsrc-id = <IMX_SC_R_FSPI_0>; clk-type = <IMX_SC_PM_CLK_PER>; power-domains = <&pd IMX_SC_R_FSPI_0>; }; fspi1_clk: clock-fspi1{ ... }; ... }; /* LPCG clocks */ lsio_lpcg_clk: lsio-lpcg-clock-controller { compatible = "fsl,imx8qxp-lpcg"; pwm0_lpcg: clock-controller@5d400000 { reg = <0x5d400000 0x10000>; #clock-cells = <1>; clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>, <&lsio_bus_clk>, <&pwm0_clk>; bit-offset = <0 4 16 20 24>; clock-output-names = "pwm0_lpcg_ipg_clk", "pwm0_lpcg_ipg_hf_clk", "pwm0_lpcg_ipg_s_clk", "pwm0_lpcg_ipg_slv_clk", "pwm0_lpcg_ipg_mstr_clk"; power-domains = <&pd IMX_SC_R_PWM_0>; status = "disabled"; }; ... }; I also have spent a lot time to investigate how TI and Samsung does. However, finally i.MX is still different and I still believe current way is better for i.MX, mainly due to below reasons: 1) IMX having separate clock controllers in HW, not shared one like others 2) IMX SoC is comprised of various HW SS (Subsystem) while others don't have. Describing clocks in DT can help a better SW architecture to describe HW. 3) Each clock is associated with a power domain. DT is the best place to indicate it. 4) Clock availability (Both SCU and LPCG) are configurable according to different HW partition configuration by SCU firmware. Defining them all in driver will cause annoying and continued churn in driver all the time when adding new SoC support. e.g. Handling availability for different SS in different SoC. Defining Clock IDs for diferent SS in different SoC for same clocks. By putting clocks in DT, we can make the clock driver completely generic and no more churn in the driver anymore in the future for adding new SoC support. It can significantly save the driver maintain effort. Last, this won't break compatibility. It's just introduce a new binding. Regards Dong Aisheng > > +- clock-output-names: Shall be the corresponding names of the outputs. > > + NOTE this property must be specified in the > same order > > + as the clock bit-offset and hw-autogate property. > > + > > +Legacy binding (DEPRECATED): > > - compatible: Should be one of: > > "fsl,imx8qxp-lpcg-adma",
Hi Stephen, > > > a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > > > b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > > > index 965cfa4..a317844 100644 > > > --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > > > +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt > > > @@ -11,6 +11,20 @@ enabled by these control bits, it might still not > > > be running based on the base resource. > > > > > > Required properties: > > > +- compatible: Should be one of: > > > + "fsl,imx8qxp-lpcg" > > > + "fsl,imx8qm-lpcg" followed by > > "fsl,imx8qxp-lpcg". > > > +- reg: Address and length of the register set. > > > +- #clock-cells: Should be 1. One LPCG supports multiple > > clocks. > > > +- clocks: Input parent clocks phandle array for each clock. > > > +- bit-offset: An integer array indicating the bit offset for each > > clock. > > > +- hw-autogate: Boolean array indicating whether supports HW > > autogate for > > > + each clock. > > > > This looks like one clk per node style of bindings which is a > > direction we don't want DT bindings to go in. It leads to a bunch of > > time parsing DT to generate clks and in general doesn't represent the > > clock controller hardware that is there. Basically, anything with 'bit-offset' > > in the binding is not going to be acceptable. > > > > This is not one clk per node but one clock controller per node which strictly > describes the HW. > > On MX8, each LPCG is a separate clock controller which can control a couple of > clock output gates for one specific device to use. Each device has a > corresponding LPCG clock controller and those LPCGs are independent with > each other with separate IO space. > > Those LPCGs are distributed in various SS (subsystems) along with device > resources. > Describing them in device tree SS dtsi doesn't seem to be an issue as it is > representing the real hardware. > > For SCU clocks, they're similar case that each SS having separate clock > controllers In HW which are managed by SCU firmware. So it seems also okay > to put them in device tree SS dtsi file, right? > > If you're concerning each node having one compatible string, how about doing > like many power domain does as below? > > Having only one compatible string in clock parent nodes. > > //LSIO SS > lsio_scu_clk: lsio-scu-clock-controller { > compatible = "fsl,imx8qxp-clock", "fsl,scu-clk"; > > fspi0_clk: clock-fspi0{ > #clock-cells = <0>; > rsrc-id = <IMX_SC_R_FSPI_0>; > clk-type = <IMX_SC_PM_CLK_PER>; > power-domains = <&pd IMX_SC_R_FSPI_0>; > }; > > fspi1_clk: clock-fspi1{ > ... > }; > ... > }; > > /* LPCG clocks */ > lsio_lpcg_clk: lsio-lpcg-clock-controller { > compatible = "fsl,imx8qxp-lpcg"; > > pwm0_lpcg: clock-controller@5d400000 { > reg = <0x5d400000 0x10000>; > #clock-cells = <1>; > clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>, > <&lsio_bus_clk>, <&pwm0_clk>; > bit-offset = <0 4 16 20 24>; > clock-output-names = "pwm0_lpcg_ipg_clk", > "pwm0_lpcg_ipg_hf_clk", > "pwm0_lpcg_ipg_s_clk", > "pwm0_lpcg_ipg_slv_clk", > "pwm0_lpcg_ipg_mstr_clk"; > power-domains = <&pd IMX_SC_R_PWM_0>; > status = "disabled"; > }; > ... > }; > > I also have spent a lot time to investigate how TI and Samsung does. However, > finally i.MX is still different and I still believe current way is better for i.MX, > mainly due to below reasons: > > 1) IMX having separate clock controllers in HW, not shared one like others > 2) IMX SoC is comprised of various HW SS (Subsystem) while others don't have. > Describing clocks in DT can help a better SW architecture to describe HW. > 3) Each clock is associated with a power domain. DT is the best place to > indicate it. > 4) Clock availability (Both SCU and LPCG) are configurable according to > different HW partition configuration by SCU firmware. > Defining them all in driver will cause annoying and continued churn in driver > all the time when adding new SoC support. > e.g. > Handling availability for different SS in different SoC. > Defining Clock IDs for diferent SS in different SoC for same clocks. > > By putting clocks in DT, we can make the clock driver completely generic and > no more churn in the driver anymore in the future for adding new SoC support. > It can significantly save the driver maintain effort. > Shawn is okay with the whole point of MX8 Arch improvement[1]. (e.g. moving clocks into DT) But we still need the your agreement on the clock part first. Could you help review if this is okay to you? [1] https://patchwork.kernel.org/cover/10824537/ Regards Dong Aisheng > Last, this won't break compatibility. It's just introduce a new binding. > > Regards > Dong Aisheng > > > > +- clock-output-names: Shall be the corresponding names of the outputs. > > > + NOTE this property must be specified in the > > same order > > > + as the clock bit-offset and hw-autogate > property. > > > + > > > +Legacy binding (DEPRECATED): > > > - compatible: Should be one of: > > > "fsl,imx8qxp-lpcg-adma",
diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt index 965cfa4..a317844 100644 --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt @@ -11,6 +11,20 @@ enabled by these control bits, it might still not be running based on the base resource. Required properties: +- compatible: Should be one of: + "fsl,imx8qxp-lpcg" + "fsl,imx8qm-lpcg" followed by "fsl,imx8qxp-lpcg". +- reg: Address and length of the register set. +- #clock-cells: Should be 1. One LPCG supports multiple clocks. +- clocks: Input parent clocks phandle array for each clock. +- bit-offset: An integer array indicating the bit offset for each clock. +- hw-autogate: Boolean array indicating whether supports HW autogate for + each clock. +- clock-output-names: Shall be the corresponding names of the outputs. + NOTE this property must be specified in the same order + as the clock bit-offset and hw-autogate property. + +Legacy binding (DEPRECATED): - compatible: Should be one of: "fsl,imx8qxp-lpcg-adma", "fsl,imx8qxp-lpcg-conn",
MX8QM and MX8QXP LPCG Clocks are mostly the same except they may reside in different subsystems across CPUs and also vary a bit on the availability. Same as SCU clock, we want to move the clock definition into device tree which can fully decouple the dependency of Clock ID definition from device tree. And no frequent changes required in clock driver any more to handle the difference. We can use the existence of clock nodes in device tree to address the device and clock availability differences across different SoCs. Cc: Rob Herring <robh@kernel.org> Cc: Stephen Boyd <sboyd@kernel.org> Cc: Shawn Guo <shawnguo@kernel.org> Cc: Sascha Hauer <kernel@pengutronix.de> Cc: Michael Turquette <mturquette@baylibre.com> Cc: devicetree@vger.kernel.org Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com> --- Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+)