[PATCH v3 01/12] clk: Always use the supplied struct clk

[Rick Chen]

Hi Lukasz

> From: Lukasz Majewski [mailto:lukma at 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 <seanga2 at gmail.com>
>
> Thank you Sean for your CCF enhancement and updating the ccf.txt documentation entry.
>
> Acked-by: Lukasz Majewski <lukma at 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: lukma at 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.