Re: [PATCH v3 01/12] clk: Always use the supplied struct clk
Hi Lukasz > From: Lukasz Majewski [mailto:lu...@denx.de] > Sent: Friday, February 07, 2020 5:22 AM > To: Sean Anderson > Cc: U-Boot Mailing List; Rick Jian-Zhi Chen(陳建志); Lukas Auer > Subject: Re: [PATCH v3 01/12] clk: Always use the supplied struct clk > > Hi Sean, > > > CCF clocks should always use the struct clock passed to their methods > > for extracting the driver-specific clock information struct. > > Previously, many functions would use the clk->dev->priv if the device > > was bound. This could cause problems with composite clocks. The > > individual clocks in a composite clock did not have the ->dev field > > filled in. This was fine, because the device-specific clock > > information would be used. However, since there was no ->dev, there > > was no way to get the parent clock. This caused the recalc_rate method > > of the CCF divider clock to fail. One option would be to use the > > clk->priv field to get the composite clock and from there get the > > appropriate parent device. However, this would tie the implementation > > to the composite clock. In general, different devices should not rely > > on the contents of ->priv from another device. > > > > The simple solution to this problem is to just always use the supplied > > struct clock. The composite clock now fills in the ->dev pointer of > > its child clocks. This allows child clocks to make calls like > > clk_get_parent() without issue. > > > > imx avoided the above problem by using a custom get_rate function with > > composite clocks. > > > > Signed-off-by: Sean Anderson > > Thank you Sean for your CCF enhancement and updating the ccf.txt > documentation entry. > > Acked-by: Lukasz Majewski > > I don't mind if RISC-V SoC maintainer pulls the whole series (including CCF > patches). OK Thanks for your review. Regards, Rick > > > --- > > Changes for v3: > > - Documented new assumptions in the CCF > > - Wrapped docs to 80 columns > > > > doc/imx/clk/ccf.txt| 63 > > +- drivers/clk/clk-composite.c| > > 8 + drivers/clk/clk-divider.c | 6 ++-- > > drivers/clk/clk-fixed-factor.c | 3 +- > > drivers/clk/clk-gate.c | 6 ++-- > > drivers/clk/clk-mux.c | 12 +++ > > drivers/clk/imx/clk-gate2.c| 4 +-- > > 7 files changed, 51 insertions(+), 51 deletions(-) > > > > diff --git a/doc/imx/clk/ccf.txt b/doc/imx/clk/ccf.txt index > > 36b60dc438..e40ac360e8 100644 > > --- a/doc/imx/clk/ccf.txt > > +++ b/doc/imx/clk/ccf.txt > > @@ -1,42 +1,37 @@ > > Introduction: > > = > > > > -This documentation entry describes the Common Clock Framework [CCF] > > -port from Linux kernel (v5.1.12) to U-Boot. > > +This documentation entry describes the Common Clock Framework [CCF] > > port from +Linux kernel (v5.1.12) to U-Boot. > > > > -This code is supposed to bring CCF to IMX based devices (imx6q, imx7 > > -imx8). Moreover, it also provides some common clock code, which would > > -allow easy porting of CCF Linux code to other platforms. > > +This code is supposed to bring CCF to IMX based devices (imx6q, imx7 > > imx8). +Moreover, it also provides some common clock code, which would > > allow easy +porting of CCF Linux code to other platforms. > > > > Design decisions: > > = > > > > -* U-Boot's driver model [DM] for clk differs from Linux CCF. The most > > - notably difference is the lack of support for hierarchical clocks > > and > > - "clock as a manager driver" (single clock DTS node acts as a > > starting > > - point for all other clocks). > > +* U-Boot's driver model [DM] for clk differs from Linux CCF. The > > most notably > > + difference is the lack of support for hierarchical clocks and > > "clock as a > > + manager driver" (single clock DTS node acts as a starting point > > for all other > > + clocks). > > > > -* The clk_get_rate() caches the previously read data if > > CLK_GET_RATE_NOCACHE > > - is not set (no need for recursive access). > > +* The clk_get_rate() caches the previously read data if > > CLK_GET_RATE_NOCACHE is > > + not set (no need for recursive access). > > > > -* On purpose the "manager" clk driver (clk-imx6q.c) is not using > > large > > - table to store pointers to clocks - e.g. > > clk[IMX6QDL_CLK_USDHC2_SEL] = > > - Instead we use udevice's linked list for the same class > > (U
Re: [PATCH v3 01/12] clk: Always use the supplied struct clk
Hi Sean, > CCF clocks should always use the struct clock passed to their methods > for extracting the driver-specific clock information struct. > Previously, many functions would use the clk->dev->priv if the device > was bound. This could cause problems with composite clocks. The > individual clocks in a composite clock did not have the ->dev field > filled in. This was fine, because the device-specific clock > information would be used. However, since there was no ->dev, there > was no way to get the parent clock. This caused the recalc_rate > method of the CCF divider clock to fail. One option would be to use > the clk->priv field to get the composite clock and from there get the > appropriate parent device. However, this would tie the implementation > to the composite clock. In general, different devices should not rely > on the contents of ->priv from another device. > > The simple solution to this problem is to just always use the > supplied struct clock. The composite clock now fills in the ->dev > pointer of its child clocks. This allows child clocks to make calls > like clk_get_parent() without issue. > > imx avoided the above problem by using a custom get_rate function > with composite clocks. > > Signed-off-by: Sean Anderson Thank you Sean for your CCF enhancement and updating the ccf.txt documentation entry. Acked-by: Lukasz Majewski I don't mind if RISC-V SoC maintainer pulls the whole series (including CCF patches). > --- > Changes for v3: > - Documented new assumptions in the CCF > - Wrapped docs to 80 columns > > doc/imx/clk/ccf.txt| 63 > +- drivers/clk/clk-composite.c| > 8 + drivers/clk/clk-divider.c | 6 ++-- > drivers/clk/clk-fixed-factor.c | 3 +- > drivers/clk/clk-gate.c | 6 ++-- > drivers/clk/clk-mux.c | 12 +++ > drivers/clk/imx/clk-gate2.c| 4 +-- > 7 files changed, 51 insertions(+), 51 deletions(-) > > diff --git a/doc/imx/clk/ccf.txt b/doc/imx/clk/ccf.txt > index 36b60dc438..e40ac360e8 100644 > --- a/doc/imx/clk/ccf.txt > +++ b/doc/imx/clk/ccf.txt > @@ -1,42 +1,37 @@ > Introduction: > = > > -This documentation entry describes the Common Clock Framework [CCF] > -port from Linux kernel (v5.1.12) to U-Boot. > +This documentation entry describes the Common Clock Framework [CCF] > port from +Linux kernel (v5.1.12) to U-Boot. > > -This code is supposed to bring CCF to IMX based devices (imx6q, imx7 > -imx8). Moreover, it also provides some common clock code, which would > -allow easy porting of CCF Linux code to other platforms. > +This code is supposed to bring CCF to IMX based devices (imx6q, imx7 > imx8). +Moreover, it also provides some common clock code, which > would allow easy +porting of CCF Linux code to other platforms. > > Design decisions: > = > > -* U-Boot's driver model [DM] for clk differs from Linux CCF. The most > - notably difference is the lack of support for hierarchical clocks > and > - "clock as a manager driver" (single clock DTS node acts as a > starting > - point for all other clocks). > +* U-Boot's driver model [DM] for clk differs from Linux CCF. The > most notably > + difference is the lack of support for hierarchical clocks and > "clock as a > + manager driver" (single clock DTS node acts as a starting point > for all other > + clocks). > > -* The clk_get_rate() caches the previously read data if > CLK_GET_RATE_NOCACHE > - is not set (no need for recursive access). > +* The clk_get_rate() caches the previously read data if > CLK_GET_RATE_NOCACHE is > + not set (no need for recursive access). > > -* On purpose the "manager" clk driver (clk-imx6q.c) is not using > large > - table to store pointers to clocks - e.g. > clk[IMX6QDL_CLK_USDHC2_SEL] = > - Instead we use udevice's linked list for the same class > (UCLASS_CLK). +* On purpose the "manager" clk driver (clk-imx6q.c) is > not using large table to > + store pointers to clocks - e.g. clk[IMX6QDL_CLK_USDHC2_SEL] = > Instead we > + use udevice's linked list for the same class (UCLASS_CLK). > >Rationale: >-- > -When porting the code as is from Linux, one would need ~1KiB of > RAM to > -store it. This is way too much if we do plan to use this driver > in SPL. > +When porting the code as is from Linux, one would need ~1KiB of > RAM to store > +it. This is way too much if we do plan to use this driver in SPL. > > * The "central" structure of this patch series is struct udevice and > its uclass_priv field contains the struct clk pointer (to the > originally created one). > > -* Up till now U-Boot's driver model (DM) CLK operates on udevice > (main > - access to clock is by udevice ops) > - In the CCF the access to struct clk (embodying pointer to *dev) is > - possible via dev_get_clk_ptr() (it is a wrapper on > dev_get_uclass_priv()). - > * To keep things simple the struct udevice's uclass_priv pointer
[PATCH v3 01/12] clk: Always use the supplied struct clk
CCF clocks should always use the struct clock passed to their methods for extracting the driver-specific clock information struct. Previously, many functions would use the clk->dev->priv if the device was bound. This could cause problems with composite clocks. The individual clocks in a composite clock did not have the ->dev field filled in. This was fine, because the device-specific clock information would be used. However, since there was no ->dev, there was no way to get the parent clock. This caused the recalc_rate method of the CCF divider clock to fail. One option would be to use the clk->priv field to get the composite clock and from there get the appropriate parent device. However, this would tie the implementation to the composite clock. In general, different devices should not rely on the contents of ->priv from another device. The simple solution to this problem is to just always use the supplied struct clock. The composite clock now fills in the ->dev pointer of its child clocks. This allows child clocks to make calls like clk_get_parent() without issue. imx avoided the above problem by using a custom get_rate function with composite clocks. Signed-off-by: Sean Anderson --- Changes for v3: - Documented new assumptions in the CCF - Wrapped docs to 80 columns doc/imx/clk/ccf.txt| 63 +- drivers/clk/clk-composite.c| 8 + drivers/clk/clk-divider.c | 6 ++-- drivers/clk/clk-fixed-factor.c | 3 +- drivers/clk/clk-gate.c | 6 ++-- drivers/clk/clk-mux.c | 12 +++ drivers/clk/imx/clk-gate2.c| 4 +-- 7 files changed, 51 insertions(+), 51 deletions(-) diff --git a/doc/imx/clk/ccf.txt b/doc/imx/clk/ccf.txt index 36b60dc438..e40ac360e8 100644 --- a/doc/imx/clk/ccf.txt +++ b/doc/imx/clk/ccf.txt @@ -1,42 +1,37 @@ Introduction: = -This documentation entry describes the Common Clock Framework [CCF] -port from Linux kernel (v5.1.12) to U-Boot. +This documentation entry describes the Common Clock Framework [CCF] port from +Linux kernel (v5.1.12) to U-Boot. -This code is supposed to bring CCF to IMX based devices (imx6q, imx7 -imx8). Moreover, it also provides some common clock code, which would -allow easy porting of CCF Linux code to other platforms. +This code is supposed to bring CCF to IMX based devices (imx6q, imx7 imx8). +Moreover, it also provides some common clock code, which would allow easy +porting of CCF Linux code to other platforms. Design decisions: = -* U-Boot's driver model [DM] for clk differs from Linux CCF. The most - notably difference is the lack of support for hierarchical clocks and - "clock as a manager driver" (single clock DTS node acts as a starting - point for all other clocks). +* U-Boot's driver model [DM] for clk differs from Linux CCF. The most notably + difference is the lack of support for hierarchical clocks and "clock as a + manager driver" (single clock DTS node acts as a starting point for all other + clocks). -* The clk_get_rate() caches the previously read data if CLK_GET_RATE_NOCACHE - is not set (no need for recursive access). +* The clk_get_rate() caches the previously read data if CLK_GET_RATE_NOCACHE is + not set (no need for recursive access). -* On purpose the "manager" clk driver (clk-imx6q.c) is not using large - table to store pointers to clocks - e.g. clk[IMX6QDL_CLK_USDHC2_SEL] = - Instead we use udevice's linked list for the same class (UCLASS_CLK). +* On purpose the "manager" clk driver (clk-imx6q.c) is not using large table to + store pointers to clocks - e.g. clk[IMX6QDL_CLK_USDHC2_SEL] = Instead we + use udevice's linked list for the same class (UCLASS_CLK). Rationale: -- -When porting the code as is from Linux, one would need ~1KiB of RAM to -store it. This is way too much if we do plan to use this driver in SPL. +When porting the code as is from Linux, one would need ~1KiB of RAM to store +it. This is way too much if we do plan to use this driver in SPL. * The "central" structure of this patch series is struct udevice and its uclass_priv field contains the struct clk pointer (to the originally created one). -* Up till now U-Boot's driver model (DM) CLK operates on udevice (main - access to clock is by udevice ops) - In the CCF the access to struct clk (embodying pointer to *dev) is - possible via dev_get_clk_ptr() (it is a wrapper on dev_get_uclass_priv()). - * To keep things simple the struct udevice's uclass_priv pointer is used to store back pointer to corresponding struct clk. However, it is possible to modify clk-uclass.c file and add there struct uc_clk_priv, which would have @@ -45,13 +40,17 @@ Design decisions: setting .per_device_auto_alloc_size = sizeof(struct uc_clk_priv)) the uclass_priv stores the pointer to struct clk. +* Non-CCF clocks do not have a pointer to a clock in clk->dev->priv. In the case + of composite
