[U-Boot] [PATCH] clk: convert API to match reset/mailbox style

Simon Glass sjg at chromium.org
Wed Jun 8 04:43:29 CEST 2016


Hi Stephen,

On 23 May 2016 at 10:47, Stephen Warren <swarren at wwwdotorg.org> wrote:
> From: Stephen Warren <swarren at nvidia.com>
>
> The following changes are made to the clock API:
> * The concept of "clocks" and "peripheral clocks" are unified; each clock
>   provider now implements a single set of clocks. This provides a simpler
>   conceptual interface to clients, and better aligns with device tree
>   clock bindings.
> * Clocks are now identified with a single "struct clk", rather than
>   requiring clients to store the clock provider device and clock identity
>   values separately. For simple clock consumers, this isolates clients
>   from internal details of the clock API.
> * clk.h is split into clk_client.h and clk_uclass.h to make it obvious
>   which parts are relevant to consumers and providers. This aligns with
>   the recently added reset and mailbox APIs.
> * clk_ops .of_xlate(), .request(), and .free() are added so providers
>   can customize these operations if needed. This also aligns with the
>   recently added reset and mailbox APIs.
> * clk_disable() is added.
> * All users of the current clock APIs are updated.
> * Sandbox clock tests are updated to exercise clock lookup via DT, and
>   clock enable/disable.
> * rkclk_get_clk() is removed and replaced with standard APIs.
>
> Buildman shows no clock-related errors for any board for which buildman
> can download a toolchain.
>
> test/py passes for sandbox (which invokes the dm clk test amongst
> others).

Sorry for the delay on this. It took more thinking and testing than I
had time for before I went away.

It looks really clean to me. Just some minor things I'd like to change.

Acked-by: Simon Glass <sjg at chromium.org>

>
> Cc: Mateusz Kulikowski <mateusz.kulikowski at gmail.com>
> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
> Cc: Purna Chandra Mandal <purna.mandal at microchip.com>
> Cc: Masahiro Yamada <yamada.masahiro at socionext.com>
> Signed-off-by: Stephen Warren <swarren at nvidia.com>
> ---
>  arch/arm/include/asm/arch-rockchip/clock.h   |  12 --
>  arch/arm/mach-rockchip/board.c               |  41 +++++-
>  arch/arm/mach-rockchip/rk3288/sdram_rk3288.c |  17 ++-
>  arch/arm/mach-snapdragon/clock-apq8016.c     |  10 +-
>  arch/arm/mach-zynq/clk.c                     |   1 -
>  arch/mips/mach-pic32/cpu.c                   |  47 +++---
>  arch/sandbox/dts/test.dts                    |  17 ++-
>  arch/sandbox/include/asm/clk.h               |  39 +++++
>  arch/sandbox/include/asm/test.h              |   9 --
>  board/microchip/pic32mzda/pic32mzda.c        |  23 ++-
>  cmd/clk.c                                    |   2 +-
>  drivers/clk/Makefile                         |   1 +
>  drivers/clk/clk-uclass.c                     | 204 +++++++++++++++++++--------
>  drivers/clk/clk_fixed_rate.c                 |  13 +-
>  drivers/clk/clk_pic32.c                      |  32 ++---
>  drivers/clk/clk_rk3036.c                     |  83 +++--------
>  drivers/clk/clk_rk3288.c                     | 141 ++++--------------
>  drivers/clk/clk_sandbox.c                    |  85 ++++++-----
>  drivers/clk/clk_sandbox_test.c               | 101 +++++++++++++
>  drivers/clk/uniphier/clk-uniphier-core.c     |  26 ++--
>  drivers/clk/uniphier/clk-uniphier-mio.c      |   1 -
>  drivers/gpio/rk_gpio.c                       |   1 -
>  drivers/i2c/rk_i2c.c                         |   8 +-
>  drivers/mmc/msm_sdhci.c                      |  15 +-
>  drivers/mmc/rockchip_dw_mmc.c                |   8 +-
>  drivers/mmc/uniphier-sd.c                    |  17 +--
>  drivers/serial/serial_msm.c                  |  15 +-
>  drivers/serial/serial_pic32.c                |   9 +-
>  drivers/spi/rk_spi.c                         |   8 +-
>  drivers/usb/host/ehci-generic.c              |  16 +--
>  drivers/video/rockchip/rk_edp.c              |  13 +-
>  drivers/video/rockchip/rk_hdmi.c             |  14 +-
>  drivers/video/rockchip/rk_lvds.c             |   1 -
>  drivers/video/rockchip/rk_vop.c              |  13 +-
>  include/clk.h                                | 132 -----------------
>  include/clk_client.h                         | 174 +++++++++++++++++++++++
>  include/clk_uclass.h                         |  95 +++++++++++++

Can we use clk.h and clk-uclass.h instead? I'm not keen on the _client
suffix. Client should be the defauilt.

>  test/dm/clk.c                                | 110 ++++++++++-----
>  38 files changed, 948 insertions(+), 606 deletions(-)
>  create mode 100644 arch/sandbox/include/asm/clk.h
>  create mode 100644 drivers/clk/clk_sandbox_test.c
>  delete mode 100644 include/clk.h
>  create mode 100644 include/clk_client.h
>  create mode 100644 include/clk_uclass.h
>
> diff --git a/arch/arm/include/asm/arch-rockchip/clock.h b/arch/arm/include/asm/arch-rockchip/clock.h
> index d66b26f18ef3..317e5128ed2b 100644
> --- a/arch/arm/include/asm/arch-rockchip/clock.h
> +++ b/arch/arm/include/asm/arch-rockchip/clock.h
> @@ -62,18 +62,6 @@ static inline u32 clk_get_divisor(ulong input_rate, uint output_rate)
>   */
>  void *rockchip_get_cru(void);
>
> -/**
> - * rkclk_get_clk() - get a pointer to a given clock
> - *
> - * This is an internal function - use outside the clock subsystem indicates
> - * that work is needed!
> - *
> - * @clk_id:    Clock requested
> - * @devp:      Returns a pointer to that clock
> - * @return 0 if OK, -ve on error
> - */
> -int rkclk_get_clk(enum rk_clk_id clk_id, struct udevice **devp);
> -
>  struct rk3288_cru;
>  struct rk3288_grf;
>

[snip]

> diff --git a/arch/sandbox/include/asm/clk.h b/arch/sandbox/include/asm/clk.h
> new file mode 100644
> index 000000000000..7fd2868f9080
> --- /dev/null
> +++ b/arch/sandbox/include/asm/clk.h
> @@ -0,0 +1,39 @@
> +/*
> + * Copyright (c) 2016, NVIDIA CORPORATION.
> + *
> + * SPDX-License-Identifier: GPL-2.0
> + */
> +
> +#ifndef __SANDBOX_CLK_H
> +#define __SANDBOX_CLK_H
> +
> +#include <common.h>
> +
> +struct udevice;
> +
> +enum {
> +       SANDBOX_CLK_ID_SPI,
> +       SANDBOX_CLK_ID_I2C,
> +
> +       SANDBOX_CLK_ID_COUNT,
> +};
> +
> +enum {
> +       SANDBOX_CLK_TEST_ID_FIXED,
> +       SANDBOX_CLK_TEST_ID_SPI,
> +       SANDBOX_CLK_TEST_ID_I2C,
> +
> +       SANDBOX_CLK_TEST_ID_COUNT,

What's the difference between these two enums? Can you add a comment to each?

> +};
> +
> +ulong sandbox_clk_query_rate(struct udevice *dev, int id);
> +int sandbox_clk_query_enable(struct udevice *dev, int id);

Can you add function comments for these?

> +
> +int sandbox_clk_test_get(struct udevice *dev);
> +ulong sandbox_clk_test_get_rate(struct udevice *dev, int id);
> +ulong sandbox_clk_test_set_rate(struct udevice *dev, int id, ulong rate);
> +int sandbox_clk_test_enable(struct udevice *dev, int id);
> +int sandbox_clk_test_disable(struct udevice *dev, int id);
> +int sandbox_clk_test_free(struct udevice *dev);

and these?

> +
> +#endif

Regards,
Simon


More information about the U-Boot mailing list