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

[Lukasz Majewski <lukma@denx.de>]

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@gmail.com>

Thank you Sean for your CCF enhancement and updating the ccf.txt
documentation entry.

Acked-by: Lukasz Majewski <lukma@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: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200206/d62c0e29/attachment.sig>