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 <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).
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
> > (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
> CONFIDENTIALITY NOTICE:
>
> This e-mail (and its attachments) may contain confidential and legally
> privileged information or information protected from disclosure. If you are
> not the intended recipient, you are hereby notified that any disclosure,
> copying, distribution, or use of the information contained herein is strictly
> prohibited. In this case, please immediately notify the sender by return
> e-mail, delete the message (and any accompanying documents) and destroy all
> printed hard copies. Thank you for your cooperation.
>
> Copyright ANDES TECHNOLOGY CORPORATION - All Rights Reserved.
