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 <sean...@gmail.com>
Thank you Sean for your CCF enhancement and updating the ccf.txt documentation entry. Acked-by: Lukasz Majewski <lu...@denx.de> 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 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 clocks, clk->dev->priv may not match clk. Drivers > should always > + use the struct clk which is passed to them, and not clk->dev->priv. > + > * It is advised to add common clock code (like already added rate > and flags) to the struct clk, which is a top level description of the > clock. > * U-Boot's driver model already provides the facility to > automatically allocate > - (via private_alloc_size) device private data (accessible via > dev->priv). > - It may look appealing to use this feature to allocate private > structures for > - CCF clk devices e.g. divider (struct clk_divider *divider) for > IMX6Q clock. > + (via private_alloc_size) device private data (accessible via > dev->priv). It > + may look appealing to use this feature to allocate private > structures for CCF > + clk devices e.g. divider (struct clk_divider *divider) for IMX6Q > clock. > The above feature had not been used for following reasons: > - The original CCF Linux kernel driver is the "manager" for clocks > - it @@ -64,21 +63,23 @@ Design decisions: > > * I've added the clk_get_parent(), which reads parent's > dev->uclass_priv to provide parent's struct clk pointer. This seems > the easiest way to get > - child/parent relationship for struct clk in U-Boot's udevice based > clocks. > + child/parent relationship for struct clk in U-Boot's udevice based > clocks. In > + the future arbitrary parents may be supported by adding a > get_parent function > + to clk_ops. > > * Linux's CCF 'struct clk_core' corresponds to U-Boot's udevice in > 'struct clk'. Clock IP block agnostic flags from 'struct clk_core' > (e.g. NOCACHE) have been > - moved from this struct one level up to 'struct clk'. > + moved from this struct one level up to 'struct clk'. Many flags are > + unimplemented at the moment. > > * For tests the new ./test/dm/clk_ccf.c and > ./drivers/clk/clk_sandbox_ccf.c files have been introduced. The > latter setups the CCF clock structure for > - sandbox by reusing, if possible, generic clock primitives - like > divier > - and mux. The former file provides code to tests this setup. > + sandbox by reusing, if possible, generic clock primitives - like > divier and > + mux. The former file provides code to tests this setup. > > For sandbox new CONFIG_SANDBOX_CLK_CCF Kconfig define has been > introduced. > - All new primitives added for new architectures must have > corresponding test > - in the two aforementioned files. > - > + All new primitives added for new architectures must have > corresponding test in > + the two aforementioned files. > > Testing (sandbox): > ================== > diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c > index a5626c33d1..d0f273d47f 100644 > --- a/drivers/clk/clk-composite.c > +++ b/drivers/clk/clk-composite.c > @@ -145,6 +145,14 @@ struct clk *clk_register_composite(struct device > *dev, const char *name, goto err; > } > > + if (composite->mux) > + composite->mux->dev = clk->dev; > + if (composite->rate) > + composite->rate->dev = clk->dev; > + if (composite->gate) > + composite->gate->dev = clk->dev; > + > + > return clk; > > err: > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index 822e09b084..bfa05f24a3 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -70,8 +70,7 @@ unsigned long divider_recalc_rate(struct clk *hw, > unsigned long parent_rate, > static ulong clk_divider_recalc_rate(struct clk *clk) > { > - struct clk_divider *divider = > to_clk_divider(clk_dev_binded(clk) ? > - dev_get_clk_ptr(clk->dev) : clk); > + struct clk_divider *divider = to_clk_divider(clk); > unsigned long parent_rate = clk_get_parent_rate(clk); > unsigned int val; > > @@ -150,8 +149,7 @@ int divider_get_val(unsigned long rate, unsigned > long parent_rate, > static ulong clk_divider_set_rate(struct clk *clk, unsigned long > rate) { > - struct clk_divider *divider = > to_clk_divider(clk_dev_binded(clk) ? > - dev_get_clk_ptr(clk->dev) : clk); > + struct clk_divider *divider = to_clk_divider(clk); > unsigned long parent_rate = clk_get_parent_rate(clk); > int value; > u32 val; > diff --git a/drivers/clk/clk-fixed-factor.c > b/drivers/clk/clk-fixed-factor.c index 711b0588bc..d2401cf440 100644 > --- a/drivers/clk/clk-fixed-factor.c > +++ b/drivers/clk/clk-fixed-factor.c > @@ -18,8 +18,7 @@ > > static ulong clk_factor_recalc_rate(struct clk *clk) > { > - struct clk_fixed_factor *fix = > - to_clk_fixed_factor(dev_get_clk_ptr(clk->dev)); > + struct clk_fixed_factor *fix = to_clk_fixed_factor(clk); > unsigned long parent_rate = clk_get_parent_rate(clk); > unsigned long long int rate; > > diff --git a/drivers/clk/clk-gate.c b/drivers/clk/clk-gate.c > index 70b8794554..b2933bc24a 100644 > --- a/drivers/clk/clk-gate.c > +++ b/drivers/clk/clk-gate.c > @@ -43,8 +43,7 @@ > */ > static void clk_gate_endisable(struct clk *clk, int enable) > { > - struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ? > - dev_get_clk_ptr(clk->dev) : clk); > + struct clk_gate *gate = to_clk_gate(clk); > int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0; > u32 reg; > > @@ -86,8 +85,7 @@ static int clk_gate_disable(struct clk *clk) > > int clk_gate_is_enabled(struct clk *clk) > { > - struct clk_gate *gate = to_clk_gate(clk_dev_binded(clk) ? > - dev_get_clk_ptr(clk->dev) : clk); > + struct clk_gate *gate = to_clk_gate(clk); > u32 reg; > > #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF) > diff --git a/drivers/clk/clk-mux.c b/drivers/clk/clk-mux.c > index 5acc0b8cbd..67b4afef28 100644 > --- a/drivers/clk/clk-mux.c > +++ b/drivers/clk/clk-mux.c > @@ -35,8 +35,7 @@ > int clk_mux_val_to_index(struct clk *clk, u32 *table, unsigned int > flags, unsigned int val) > { > - struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ? > - dev_get_clk_ptr(clk->dev) : clk); > + struct clk_mux *mux = to_clk_mux(clk); > int num_parents = mux->num_parents; > > if (table) { > @@ -79,8 +78,7 @@ unsigned int clk_mux_index_to_val(u32 *table, > unsigned int flags, u8 index) > u8 clk_mux_get_parent(struct clk *clk) > { > - struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ? > - dev_get_clk_ptr(clk->dev) : clk); > + struct clk_mux *mux = to_clk_mux(clk); > u32 val; > > #if CONFIG_IS_ENABLED(SANDBOX_CLK_CCF) > @@ -97,8 +95,7 @@ u8 clk_mux_get_parent(struct clk *clk) > static int clk_fetch_parent_index(struct clk *clk, > struct clk *parent) > { > - struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ? > - dev_get_clk_ptr(clk->dev) : clk); > + struct clk_mux *mux = to_clk_mux(clk); > > int i; > > @@ -115,8 +112,7 @@ static int clk_fetch_parent_index(struct clk *clk, > > static int clk_mux_set_parent(struct clk *clk, struct clk *parent) > { > - struct clk_mux *mux = to_clk_mux(clk_dev_binded(clk) ? > - dev_get_clk_ptr(clk->dev) : clk); > + struct clk_mux *mux = to_clk_mux(clk); > int index; > u32 val; > u32 reg; > diff --git a/drivers/clk/imx/clk-gate2.c b/drivers/clk/imx/clk-gate2.c > index 1b9db6e791..e32c0cb53e 100644 > --- a/drivers/clk/imx/clk-gate2.c > +++ b/drivers/clk/imx/clk-gate2.c > @@ -37,7 +37,7 @@ struct clk_gate2 { > > static int clk_gate2_enable(struct clk *clk) > { > - struct clk_gate2 *gate = > to_clk_gate2(dev_get_clk_ptr(clk->dev)); > + struct clk_gate2 *gate = to_clk_gate2(clk); > u32 reg; > > reg = readl(gate->reg); > @@ -50,7 +50,7 @@ static int clk_gate2_enable(struct clk *clk) > > static int clk_gate2_disable(struct clk *clk) > { > - struct clk_gate2 *gate = > to_clk_gate2(dev_get_clk_ptr(clk->dev)); > + struct clk_gate2 *gate = to_clk_gate2(clk); > u32 reg; > > reg = readl(gate->reg); Best regards, Lukasz Majewski -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lu...@denx.de
pgpTN_MNlz82c.pgp
Description: OpenPGP digital signature