[U-Boot,v3,00/11] clk: Port Linux common clock framework [CCF] to U-boot (tag: 5.0-rc3)

This patch series brings the files from Linux kernel to provide clocks support
as it is used on the Linux kernel with common clock framework [CCF] setup.

This series also fixes several problems with current clocks and provides
sandbox tests for functions addded to clk-uclass.c file.

Design decisions/issues:
=========================

- U-boot's 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() now caches the previously read data (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).
The 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.

- The "central" structure of this patch series is struct udevice and its
driver_data field contains the struct clk pointer (to the originally created
one).

- Up till now U-boot's driver model's CLK operates on udevice (main access to
  clock is by udevice ops)
  In the CCF the access to struct clk (comprising pointer to *dev) is
  possible via dev_get_driver_data()

  Storing back pointer (from udevice to struct clk) as driver_data is a
  convention for CCF.

- I could use *private_alloc_size to allocate driver's 'private"
  structures (dev->priv) for e.g. divider (struct clk_divider *divider)
  for IMX6Q clock, but this would change the original structure of the CCF code.

The question is if it would be better to use private_alloc_size (and
dev->private) or stay with driver_data.
The former requires some rewritting in CCF original code (to remove
(c)malloc, etc), but comply with u-boot DM. The latter allows re-using the
CCF code as is, but introduces some convention special for CCF (I'm not
sure thought if dev->priv is NOT another convention as well).

- I've added the clk_get_parent(), which reads parent's dev->driver_data 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.

- For tests I had to "emulate" CCF code structure to test functionality of clk_get_parent_rate()
  and clk_get_by_id(). Those functions will not work properly with
  "standard" (i.e. non CCF) clock setup(with not set dev->driver_data to
  struct clk).

Repository:
https://github.com/lmajewski/u-boot-dfu/commits/CCF-v3


Changes in v3:
- New patch
- The rate information is now cached into struct clk field
- The clk_get_parent() is used to get pointer to the parent struct clk
- Replace -ENODEV with -ENOENT
- Use **clkp instead of **c
- New patch
- New patch

Lukasz Majewski (11):
  dm: Fix documentation entry as there is no UCLASS_CLOCK uclass
  cmd: Do not show frequency for clocks which .get_rate() return error
  clk: Remove clock ID check in .get_rate() of clk_fixed_*
  clk: Extend struct clk to provide information regarding clock rate
  clk: Provide struct clk for fixed rate clock (clk_fixed_rate.c)
  dm: clk: Define clk_get_parent() for clk operations
  dm: clk: Define clk_get_parent_rate() for clk operations
  dm: clk: Define clk_get_by_id() for clk operations
  clk: test: Provide unit test for clk_get_by_id() method
  clk: test: Provide unit test for clk_get_parent_rate() method
  clk: Port Linux common clock framework [CCF] for imx6q to U-boot (tag:
    5.0-rc3)

 arch/sandbox/include/asm/clk.h |  16 ++++
 cmd/clk.c                      |   5 +-
 drivers/clk/Kconfig            |  14 ++++
 drivers/clk/Makefile           |   2 +
 drivers/clk/clk-divider.c      | 148 ++++++++++++++++++++++++++++++++++
 drivers/clk/clk-fixed-factor.c |  87 ++++++++++++++++++++
 drivers/clk/clk-mux.c          | 164 +++++++++++++++++++++++++++++++++++++
 drivers/clk/clk-uclass.c       |  59 ++++++++++++++
 drivers/clk/clk.c              |  56 +++++++++++++
 drivers/clk/clk_fixed_factor.c |   3 -
 drivers/clk/clk_fixed_rate.c   |   8 +-
 drivers/clk/clk_sandbox_test.c |  49 +++++++++++
 drivers/clk/imx/Kconfig        |   9 +++
 drivers/clk/imx/Makefile       |   2 +
 drivers/clk/imx/clk-gate2.c    | 113 ++++++++++++++++++++++++++
 drivers/clk/imx/clk-imx6q.c    | 179 +++++++++++++++++++++++++++++++++++++++++
 drivers/clk/imx/clk-pfd.c      |  91 +++++++++++++++++++++
 drivers/clk/imx/clk-pllv3.c    |  83 +++++++++++++++++++
 drivers/clk/imx/clk.h          |  75 +++++++++++++++++
 include/clk.h                  |  33 +++++++-
 include/linux/clk-provider.h   |  94 ++++++++++++++++++++++
 test/dm/clk.c                  |   4 +-
 22 files changed, 1285 insertions(+), 9 deletions(-)
 create mode 100644 drivers/clk/clk-divider.c
 create mode 100644 drivers/clk/clk-fixed-factor.c
 create mode 100644 drivers/clk/clk-mux.c
 create mode 100644 drivers/clk/clk.c
 create mode 100644 drivers/clk/imx/clk-gate2.c
 create mode 100644 drivers/clk/imx/clk-imx6q.c
 create mode 100644 drivers/clk/imx/clk-pfd.c
 create mode 100644 drivers/clk/imx/clk-pllv3.c
 create mode 100644 drivers/clk/imx/clk.h
 create mode 100644 include/linux/clk-provider.h

Dear All,

> This patch series brings the files from Linux kernel to provide
> clocks support as it is used on the Linux kernel with common clock
> framework [CCF] setup.
> 
> This series also fixes several problems with current clocks and
> provides sandbox tests for functions addded to clk-uclass.c file.
> 
> Design decisions/issues:
> =========================
> 
> - U-boot's 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() now caches the previously read data (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). The 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.
> 
> - The "central" structure of this patch series is struct udevice and
> its driver_data field contains the struct clk pointer (to the
> originally created one).
> 
> - Up till now U-boot's driver model's CLK operates on udevice (main
> access to clock is by udevice ops)
>   In the CCF the access to struct clk (comprising pointer to *dev) is
>   possible via dev_get_driver_data()
> 
>   Storing back pointer (from udevice to struct clk) as driver_data is
> a convention for CCF.
> 
> - I could use *private_alloc_size to allocate driver's 'private"
>   structures (dev->priv) for e.g. divider (struct clk_divider
> *divider) for IMX6Q clock, but this would change the original
> structure of the CCF code.
> 
> The question is if it would be better to use private_alloc_size (and
> dev->private) or stay with driver_data.
> The former requires some rewritting in CCF original code (to remove
> (c)malloc, etc), but comply with u-boot DM. The latter allows
> re-using the CCF code as is, but introduces some convention special
> for CCF (I'm not sure thought if dev->priv is NOT another convention
> as well).
> 
> - I've added the clk_get_parent(), which reads parent's
> dev->driver_data 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.
> 
> - For tests I had to "emulate" CCF code structure to test
> functionality of clk_get_parent_rate() and clk_get_by_id(). Those
> functions will not work properly with "standard" (i.e. non CCF) clock
> setup(with not set dev->driver_data to struct clk).
> 

I would be very grateful for comments regarding described above design
decisions.

Thanks in advance.

> Repository:
> https://github.com/lmajewski/u-boot-dfu/commits/CCF-v3
> 
> 
> Changes in v3:
> - New patch
> - The rate information is now cached into struct clk field
> - The clk_get_parent() is used to get pointer to the parent struct clk
> - Replace -ENODEV with -ENOENT
> - Use **clkp instead of **c
> - New patch
> - New patch
> 
> Lukasz Majewski (11):
>   dm: Fix documentation entry as there is no UCLASS_CLOCK uclass
>   cmd: Do not show frequency for clocks which .get_rate() return error
>   clk: Remove clock ID check in .get_rate() of clk_fixed_*
>   clk: Extend struct clk to provide information regarding clock rate
>   clk: Provide struct clk for fixed rate clock (clk_fixed_rate.c)
>   dm: clk: Define clk_get_parent() for clk operations
>   dm: clk: Define clk_get_parent_rate() for clk operations
>   dm: clk: Define clk_get_by_id() for clk operations
>   clk: test: Provide unit test for clk_get_by_id() method
>   clk: test: Provide unit test for clk_get_parent_rate() method
>   clk: Port Linux common clock framework [CCF] for imx6q to U-boot
> (tag: 5.0-rc3)
> 
>  arch/sandbox/include/asm/clk.h |  16 ++++
>  cmd/clk.c                      |   5 +-
>  drivers/clk/Kconfig            |  14 ++++
>  drivers/clk/Makefile           |   2 +
>  drivers/clk/clk-divider.c      | 148
> ++++++++++++++++++++++++++++++++++ drivers/clk/clk-fixed-factor.c |
> 87 ++++++++++++++++++++ drivers/clk/clk-mux.c          | 164
> +++++++++++++++++++++++++++++++++++++ drivers/clk/clk-uclass.c
> |  59 ++++++++++++++ drivers/clk/clk.c              |  56
> +++++++++++++ drivers/clk/clk_fixed_factor.c |   3 -
>  drivers/clk/clk_fixed_rate.c   |   8 +-
>  drivers/clk/clk_sandbox_test.c |  49 +++++++++++
>  drivers/clk/imx/Kconfig        |   9 +++
>  drivers/clk/imx/Makefile       |   2 +
>  drivers/clk/imx/clk-gate2.c    | 113 ++++++++++++++++++++++++++
>  drivers/clk/imx/clk-imx6q.c    | 179
> +++++++++++++++++++++++++++++++++++++++++
> drivers/clk/imx/clk-pfd.c      |  91 +++++++++++++++++++++
> drivers/clk/imx/clk-pllv3.c    |  83 +++++++++++++++++++
> drivers/clk/imx/clk.h          |  75 +++++++++++++++++
> include/clk.h                  |  33 +++++++-
> include/linux/clk-provider.h   |  94 ++++++++++++++++++++++
> test/dm/clk.c                  |   4 +- 22 files changed, 1285
> insertions(+), 9 deletions(-) create mode 100644
> drivers/clk/clk-divider.c create mode 100644
> drivers/clk/clk-fixed-factor.c create mode 100644
> drivers/clk/clk-mux.c create mode 100644 drivers/clk/clk.c
>  create mode 100644 drivers/clk/imx/clk-gate2.c
>  create mode 100644 drivers/clk/imx/clk-imx6q.c
>  create mode 100644 drivers/clk/imx/clk-pfd.c
>  create mode 100644 drivers/clk/imx/clk-pllv3.c
>  create mode 100644 drivers/clk/imx/clk.h
>  create mode 100644 include/linux/clk-provider.h
> 




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@denx.de

Hi Lukasz,

On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski <lukma@denx.de> wrote:
>
> This patch series brings the files from Linux kernel to provide clocks support
> as it is used on the Linux kernel with common clock framework [CCF] setup.
>
> This series also fixes several problems with current clocks and provides
> sandbox tests for functions addded to clk-uclass.c file.
>
> Design decisions/issues:
> =========================
>
> - U-boot's 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() now caches the previously read data (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).
> The 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.
>
> - The "central" structure of this patch series is struct udevice and its
> driver_data field contains the struct clk pointer (to the originally created
> one).
>
> - Up till now U-boot's driver model's CLK operates on udevice (main access to
>   clock is by udevice ops)
>   In the CCF the access to struct clk (comprising pointer to *dev) is
>   possible via dev_get_driver_data()
>
>   Storing back pointer (from udevice to struct clk) as driver_data is a
>   convention for CCF.

Ick. Why not use uclass-private data to store this, since every
UCLASS_CLK device can have a parent.

>
> - I could use *private_alloc_size to allocate driver's 'private"
>   structures (dev->priv) for e.g. divider (struct clk_divider *divider)
>   for IMX6Q clock, but this would change the original structure of the CCF code.
>
> The question is if it would be better to use private_alloc_size (and
> dev->private) or stay with driver_data.
> The former requires some rewritting in CCF original code (to remove
> (c)malloc, etc), but comply with u-boot DM. The latter allows re-using the
> CCF code as is, but introduces some convention special for CCF (I'm not
> sure thought if dev->priv is NOT another convention as well).

Yes I would like to avoid malloc() calls in drivers and use the
in-built mechanism.

>
> - I've added the clk_get_parent(), which reads parent's dev->driver_data 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.
>
> - For tests I had to "emulate" CCF code structure to test functionality of clk_get_parent_rate()
>   and clk_get_by_id(). Those functions will not work properly with
>   "standard" (i.e. non CCF) clock setup(with not set dev->driver_data to
>   struct clk).

Well I think we need a better approach for that anywat. driver_data is
used for getting something from the DT.

Regards,
Simon

Hi Simon,

This is not the newest patch set version of CCF (v3 vs. v4), but the
comments/issues apply.

> Hi Lukasz,
> 
> On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski <lukma@denx.de> wrote:
> >
> > This patch series brings the files from Linux kernel to provide
> > clocks support as it is used on the Linux kernel with common clock
> > framework [CCF] setup.
> >
> > This series also fixes several problems with current clocks and
> > provides sandbox tests for functions addded to clk-uclass.c file.
> >
> > Design decisions/issues:
> > =========================
> >
> > - U-boot's 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() now caches the previously read data (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). The 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.
> >
> > - The "central" structure of this patch series is struct udevice
> > and its driver_data field contains the struct clk pointer (to the
> > originally created one).
> >
> > - Up till now U-boot's driver model's CLK operates on udevice (main
> > access to clock is by udevice ops)
> >   In the CCF the access to struct clk (comprising pointer to *dev)
> > is possible via dev_get_driver_data()
> >
> >   Storing back pointer (from udevice to struct clk) as driver_data
> > is a convention for CCF.  
> 
> Ick. Why not use uclass-private data to store this, since every
> UCLASS_CLK device can have a parent.

The "private_data" field would be also a good place to store the
back pointer from udevice to struct clk [*]. The problem with CCF and
udevice's priv pointer is explained just below:

> 
> >
> > - I could use *private_alloc_size to allocate driver's 'private"
> >   structures (dev->priv) for e.g. divider (struct clk_divider
> > *divider) for IMX6Q clock, but this would change the original
> > structure of the CCF code.

The original Linux's CCF code for iMX relies on using kmalloc
internally:

https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/imx/clk-gate2.c#L139
https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk-divider.c#L471

By using driver_data I've avoided the need to make more changes to the
original Linux code.

I could use udevice's priv with automatic data allocation but then the
CCF ported code would require more changes and considering the (from
the outset) need to "fit" this code into U-Boot's DM, it drives away
from the original Linux code.


> >
> > The question is if it would be better to use private_alloc_size (and
> > dev->private) or stay with driver_data.
> > The former requires some rewritting in CCF original code (to remove
> > (c)malloc, etc), but comply with u-boot DM. The latter allows
> > re-using the CCF code as is, but introduces some convention special
> > for CCF (I'm not sure thought if dev->priv is NOT another
> > convention as well).  
> 
> Yes I would like to avoid malloc() calls in drivers and use the
> in-built mechanism.

I see your point. 

If the community agrees - I can rewrite the code to use such approach
(but issues pointed out in [*] still apply).

> 
> >
> > - I've added the clk_get_parent(), which reads parent's
> > dev->driver_data 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.
> >
> > - For tests I had to "emulate" CCF code structure to test
> > functionality of clk_get_parent_rate() and clk_get_by_id(). Those
> > functions will not work properly with "standard" (i.e. non CCF)
> > clock setup(with not set dev->driver_data to struct clk).  
> 
> Well I think we need a better approach for that anywat. driver_data is
> used for getting something from the DT.

Maybe the name (driver_data) was a bit misleading then. For CCF it
stores the back pointer to struct clk (as in fact it is a CCF's "driver
data"). 



NOTE:

[*] - I do have a hard time to understand how struct clk shall work
with struct udevice?

In Linux or Barebox the struct clk is the "main" structure to hold the
clock management data (like freq, ops, flags, parent/sibling relation,
etc).

A side observation - we now have three different implementations of
struct clk in U-Boot :-) (two of which have *ops inside :-) )

In the case of U-Boot's DM (./include/clk.h) it only has a _pointer_ to
udevice (which means that I cannot get the struct clk easily from
udevice with container_of()). The struct udevice has instead the *ops
and *parent pointer (to another udevice).

Two problems:

- Linux CCF code uses massively "struct clk" to handle clock operations
  (but not udevice)

- There is no clear idea of how to implement 
struct clk *clk_get_parent(struct clk *) in U-Boot.

The reason is that we lack clear information about which udevice's data
field shall be used to store the back pointer from udevice to struct
clk.

Any hints and ideas are more than welcome.

> 
> Regards,
> Simon




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@denx.de

Hi Lukasz,

On Sat, 18 May 2019 at 15:28, Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Simon,
>
> This is not the newest patch set version of CCF (v3 vs. v4), but the
> comments/issues apply.
>
> > Hi Lukasz,
> >
> > On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski <lukma@denx.de> wrote:
> > >
> > > This patch series brings the files from Linux kernel to provide
> > > clocks support as it is used on the Linux kernel with common clock
> > > framework [CCF] setup.
> > >
> > > This series also fixes several problems with current clocks and
> > > provides sandbox tests for functions addded to clk-uclass.c file.
> > >
> > > Design decisions/issues:
> > > =========================
> > >
> > > - U-boot's 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() now caches the previously read data (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). The 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.
> > >
> > > - The "central" structure of this patch series is struct udevice
> > > and its driver_data field contains the struct clk pointer (to the
> > > originally created one).
> > >
> > > - Up till now U-boot's driver model's CLK operates on udevice (main
> > > access to clock is by udevice ops)
> > >   In the CCF the access to struct clk (comprising pointer to *dev)
> > > is possible via dev_get_driver_data()
> > >
> > >   Storing back pointer (from udevice to struct clk) as driver_data
> > > is a convention for CCF.
> >
> > Ick. Why not use uclass-private data to store this, since every
> > UCLASS_CLK device can have a parent.
>
> The "private_data" field would be also a good place to store the
> back pointer from udevice to struct clk [*]. The problem with CCF and
> udevice's priv pointer is explained just below:
>
> >
> > >
> > > - I could use *private_alloc_size to allocate driver's 'private"
> > >   structures (dev->priv) for e.g. divider (struct clk_divider
> > > *divider) for IMX6Q clock, but this would change the original
> > > structure of the CCF code.
>
> The original Linux's CCF code for iMX relies on using kmalloc
> internally:
>
> https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/imx/clk-gate2.c#L139
> https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk-divider.c#L471
>
> By using driver_data I've avoided the need to make more changes to the
> original Linux code.
>
> I could use udevice's priv with automatic data allocation but then the
> CCF ported code would require more changes and considering the (from
> the outset) need to "fit" this code into U-Boot's DM, it drives away
> from the original Linux code.

Is the main change the need to cast driver_data? Perhaps that could be
hidden in a helper function/macro, so that in U-Boot it can hide the
use of (struct clk_uc_priv *)dev_get_uclass_priv(clk->dev))>parent  ?

>
>
> > >
> > > The question is if it would be better to use private_alloc_size (and
> > > dev->private) or stay with driver_data.
> > > The former requires some rewritting in CCF original code (to remove
> > > (c)malloc, etc), but comply with u-boot DM. The latter allows
> > > re-using the CCF code as is, but introduces some convention special
> > > for CCF (I'm not sure thought if dev->priv is NOT another
> > > convention as well).
> >
> > Yes I would like to avoid malloc() calls in drivers and use the
> > in-built mechanism.
>
> I see your point.
>
> If the community agrees - I can rewrite the code to use such approach
> (but issues pointed out in [*] still apply).
>
> >
> > >
> > > - I've added the clk_get_parent(), which reads parent's
> > > dev->driver_data 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.
> > >
> > > - For tests I had to "emulate" CCF code structure to test
> > > functionality of clk_get_parent_rate() and clk_get_by_id(). Those
> > > functions will not work properly with "standard" (i.e. non CCF)
> > > clock setup(with not set dev->driver_data to struct clk).
> >
> > Well I think we need a better approach for that anywat. driver_data is
> > used for getting something from the DT.
>
> Maybe the name (driver_data) was a bit misleading then. For CCF it
> stores the back pointer to struct clk (as in fact it is a CCF's "driver
> data").

Well it seems like a hack to me. Perhaps there is a good reason for it
in Linux? Or is it just convenience?

>
>
>
> NOTE:
>
> [*] - I do have a hard time to understand how struct clk shall work
> with struct udevice?
>
> In Linux or Barebox the struct clk is the "main" structure to hold the
> clock management data (like freq, ops, flags, parent/sibling relation,
> etc).

Yes U-Boot has a uniform struct udevice for every device and struct
uclass for every class.

But the interesting thing here is that clocks have their own
parent/sibling relationships, quite independent from the device tree.

>
> A side observation - we now have three different implementations of
> struct clk in U-Boot :-) (two of which have *ops inside :-) )

Oh dear.

The broadcom iMX ones needs to be converted.

>
> In the case of U-Boot's DM (./include/clk.h) it only has a _pointer_ to
> udevice (which means that I cannot get the struct clk easily from
> udevice with container_of()). The struct udevice has instead the *ops
> and *parent pointer (to another udevice).

Yes that's correct. The struct clk is actually a handle to the clock,
and includes an ID number.

>
> Two problems:
>
> - Linux CCF code uses massively "struct clk" to handle clock operations
>   (but not udevice)

OK.

>
> - There is no clear idea of how to implement
> struct clk *clk_get_parent(struct clk *) in U-Boot.

As above, it seems that this might need to be implemented. I don't
think the DM parent/child relationships are good enough for clk, since
they are not aware of the ID.

>
> The reason is that we lack clear information about which udevice's data
> field shall be used to store the back pointer from udevice to struct
> clk.
>
> Any hints and ideas are more than welcome.

I think it would be good to get Stephen Warren's thoughts on this as
he made the change to introduce struct clk.

But at present clk_set_parent() is implemented by calling into the
driver. The uclass itself does not maintain information about what is
a parent of what.

Do we *need* to maintain this information in the uclass?

I think it would be prohibitively expensive to separate out each
individual clock into a separate device (udevice), but that would
work.

The only other option I see is to create a sibling list and parent
pointer inside struct clk.

I suspect this will affect power domain also, although we don't have that yet.

Do you think there is a case for building this into DM itself, such
that devices can have a list of IDs for each device, each with
independent parent/child relationships?

Regards,
Simon

Hi Simon,

> Hi Lukasz,
> 
> On Sat, 18 May 2019 at 15:28, Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Simon,
> >
> > This is not the newest patch set version of CCF (v3 vs. v4), but the
> > comments/issues apply.
> >  
> > > Hi Lukasz,
> > >
> > > On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > > >
> > > > This patch series brings the files from Linux kernel to provide
> > > > clocks support as it is used on the Linux kernel with common
> > > > clock framework [CCF] setup.
> > > >
> > > > This series also fixes several problems with current clocks and
> > > > provides sandbox tests for functions addded to clk-uclass.c
> > > > file.
> > > >
> > > > Design decisions/issues:
> > > > =========================
> > > >
> > > > - U-boot's 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() now caches the previously read data (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). The 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.
> > > >
> > > > - The "central" structure of this patch series is struct udevice
> > > > and its driver_data field contains the struct clk pointer (to
> > > > the originally created one).
> > > >
> > > > - Up till now U-boot's driver model's CLK operates on udevice
> > > > (main access to clock is by udevice ops)
> > > >   In the CCF the access to struct clk (comprising pointer to
> > > > *dev) is possible via dev_get_driver_data()
> > > >
> > > >   Storing back pointer (from udevice to struct clk) as
> > > > driver_data is a convention for CCF.  
> > >
> > > Ick. Why not use uclass-private data to store this, since every
> > > UCLASS_CLK device can have a parent.  
> >
> > The "private_data" field would be also a good place to store the
> > back pointer from udevice to struct clk [*]. The problem with CCF
> > and udevice's priv pointer is explained just below:
> >  
> > >  
> > > >
> > > > - I could use *private_alloc_size to allocate driver's 'private"
> > > >   structures (dev->priv) for e.g. divider (struct clk_divider
> > > > *divider) for IMX6Q clock, but this would change the original
> > > > structure of the CCF code.  
> >
> > The original Linux's CCF code for iMX relies on using kmalloc
> > internally:
> >
> > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/imx/clk-gate2.c#L139
> > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk-divider.c#L471
> >
> > By using driver_data I've avoided the need to make more changes to
> > the original Linux code.
> >
> > I could use udevice's priv with automatic data allocation but then
> > the CCF ported code would require more changes and considering the
> > (from the outset) need to "fit" this code into U-Boot's DM, it
> > drives away from the original Linux code.  
> 
> Is the main change the need to cast driver_data? 

The main change would be to remove the per clock device memory
allocation code (with exit paths) from the original CCF code.

This shall not be so difficult.

> Perhaps that could be
> hidden in a helper function/macro, so that in U-Boot it can hide the
> use of (struct clk_uc_priv *)dev_get_uclass_priv(clk->dev))>parent  ?

Helper function would help to some extend as we operate on structures
similar to:

struct clk_gate2 {
	struct clk clk;
	void __iomem	*reg;
	u8		bit_idx;
	u8		cgr_val;
	u8		flags;
};

The helper would return struct clk's address which is the same as
struct's clk_gate2 (this is assured by C standard).

> 
> >
> >  
> > > >
> > > > The question is if it would be better to use private_alloc_size
> > > > (and dev->private) or stay with driver_data.
> > > > The former requires some rewritting in CCF original code (to
> > > > remove (c)malloc, etc), but comply with u-boot DM. The latter
> > > > allows re-using the CCF code as is, but introduces some
> > > > convention special for CCF (I'm not sure thought if dev->priv
> > > > is NOT another convention as well).  
> > >
> > > Yes I would like to avoid malloc() calls in drivers and use the
> > > in-built mechanism.  
> >
> > I see your point.
> >
> > If the community agrees - I can rewrite the code to use such
> > approach (but issues pointed out in [*] still apply).
> >  
> > >  
> > > >
> > > > - I've added the clk_get_parent(), which reads parent's
> > > > dev->driver_data 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.
> > > >
> > > > - For tests I had to "emulate" CCF code structure to test
> > > > functionality of clk_get_parent_rate() and clk_get_by_id().
> > > > Those functions will not work properly with "standard" (i.e.
> > > > non CCF) clock setup(with not set dev->driver_data to struct
> > > > clk).  
> > >
> > > Well I think we need a better approach for that anywat.
> > > driver_data is used for getting something from the DT.  
> >
> > Maybe the name (driver_data) was a bit misleading then. For CCF it
> > stores the back pointer to struct clk (as in fact it is a CCF's
> > "driver data").  
> 
> Well it seems like a hack to me. Perhaps there is a good reason for it
> in Linux?

In Linux there is another approach - namely the struct clk (which is
the main struct for clock management) has pointer to struct clk_core,
which has pointer to parent(s).

https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk.c#L43

In the case of U-Boot - the CCF wants to work on struct clk, but the
_main_ data structure for U-Boot is struct udevice. Hence the need to
have a back pointer (or force struct clk to have NOT pointer to udevice,
but the udevice itself - then container_of would then do the trick).

> Or is it just convenience?

As stated above - Linux all necessary information has accessible from
struct clk.

> 
> >
> >
> >
> > NOTE:
> >
> > [*] - I do have a hard time to understand how struct clk shall work
> > with struct udevice?
> >
> > In Linux or Barebox the struct clk is the "main" structure to hold
> > the clock management data (like freq, ops, flags, parent/sibling
> > relation, etc).  
> 
> Yes U-Boot has a uniform struct udevice for every device and struct
> uclass for every class.
> 
> But the interesting thing here is that clocks have their own
> parent/sibling relationships, quite independent from the device tree.

But there would be no harm if we could re-use udevice for it. In the
current CCF (v4) patch set each clk IP block (like mux or gate) is
modelled as udevice:

https://pastebin.com/uVmwv5FT

> 
> >
> > A side observation - we now have three different implementations of
> > struct clk in U-Boot :-) (two of which have *ops inside :-) )  
> 
> Oh dear.
> 
> The broadcom iMX ones needs to be converted.
> 
> >
> > In the case of U-Boot's DM (./include/clk.h) it only has a
> > _pointer_ to udevice (which means that I cannot get the struct clk
> > easily from udevice with container_of()). The struct udevice has
> > instead the *ops and *parent pointer (to another udevice).  
> 
> Yes that's correct. The struct clk is actually a handle to the clock,
> and includes an ID number.

You mean the ID number of the clock ?

> 
> >
> > Two problems:
> >
> > - Linux CCF code uses massively "struct clk" to handle clock
> > operations (but not udevice)  
> 
> OK.
> 
> >
> > - There is no clear idea of how to implement
> > struct clk *clk_get_parent(struct clk *) in U-Boot.  
> 
> As above, it seems that this might need to be implemented. I don't
> think the DM parent/child relationships are good enough for clk, since
> they are not aware of the ID.
> 
> >
> > The reason is that we lack clear information about which udevice's
> > data field shall be used to store the back pointer from udevice to
> > struct clk.
> >
> > Any hints and ideas are more than welcome.  
> 
> I think it would be good to get Stephen Warren's thoughts on this as
> he made the change to introduce struct clk.

Ok.

> 
> But at present clk_set_parent() is implemented by calling into the
> driver. The uclass itself does not maintain information about what is
> a parent of what.

Linux struct clk has easy access to its parent's struct clk. This is
what is missing in U-Boot's DM.

> 
> Do we *need* to maintain this information in the uclass?

I think that we don't need. It would be enough to modify struct clk to
has struct udevice embedded in it (as Linux has struct clk_core), not
the pointer. Then we can use container_of to get clock and re-use
struct udevice's parent pointer (and maybe UCLASS_CLK list of devices).

> 
> I think it would be prohibitively expensive to separate out each
> individual clock into a separate device (udevice), but that would
> work.

This is the approach as I use now in CCF v4:
https://pastebin.com/uVmwv5FT

It is expensive but logically correct as each mux, gate, pll is the
other CLK IP block (device).

> 
> The only other option I see is to create a sibling list and parent
> pointer inside struct clk.

This would be the approach similar to Linux kernel approach.

However, I don't know what were original needs of struct clk (as it did
not have it). Maybe Stephen can shed some light on it?

> 
> I suspect this will affect power domain also, although we don't have
> that yet.

iMX8 has some clocks which needs to be always recalculated as they
depend on power domains which can be disabled.

> 
> Do you think there is a case for building this into DM itself, such
> that devices can have a list of IDs for each device, each with
> independent parent/child relationships?

For iMX6Q the ID of the clock is used to get proper clock in drivers
(or from DTS). All clock's udevices are stored into UCLASS_CLK list.
With the ID we get proper udevice and from it and via driver_data the
struct clk, which is then used in the CCF code to operate on clock
devices (PLL, gate, mux, etc).

I simply re-used the DM facilities (only missing is the back pointer
from udevice to struct clk). 

> 
> Regards,
> Simon




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@denx.de

Hi Lukasz,

On Sun, 19 May 2019 at 15:03, Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Simon,
>
> > Hi Lukasz,
> >
> > On Sat, 18 May 2019 at 15:28, Lukasz Majewski <lukma@denx.de> wrote:
> > >
> > > Hi Simon,
> > >
> > > This is not the newest patch set version of CCF (v3 vs. v4), but the
> > > comments/issues apply.
> > >
> > > > Hi Lukasz,
> > > >
> > > > On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski <lukma@denx.de>
> > > > wrote:
> > > > >
> > > > > This patch series brings the files from Linux kernel to provide
> > > > > clocks support as it is used on the Linux kernel with common
> > > > > clock framework [CCF] setup.
> > > > >
> > > > > This series also fixes several problems with current clocks and
> > > > > provides sandbox tests for functions addded to clk-uclass.c
> > > > > file.
> > > > >
> > > > > Design decisions/issues:
> > > > > =========================
> > > > >
> > > > > - U-boot's 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() now caches the previously read data (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). The 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.
> > > > >
> > > > > - The "central" structure of this patch series is struct udevice
> > > > > and its driver_data field contains the struct clk pointer (to
> > > > > the originally created one).
> > > > >
> > > > > - Up till now U-boot's driver model's CLK operates on udevice
> > > > > (main access to clock is by udevice ops)
> > > > >   In the CCF the access to struct clk (comprising pointer to
> > > > > *dev) is possible via dev_get_driver_data()
> > > > >
> > > > >   Storing back pointer (from udevice to struct clk) as
> > > > > driver_data is a convention for CCF.
> > > >
> > > > Ick. Why not use uclass-private data to store this, since every
> > > > UCLASS_CLK device can have a parent.
> > >
> > > The "private_data" field would be also a good place to store the
> > > back pointer from udevice to struct clk [*]. The problem with CCF
> > > and udevice's priv pointer is explained just below:
> > >
> > > >
> > > > >
> > > > > - I could use *private_alloc_size to allocate driver's 'private"
> > > > >   structures (dev->priv) for e.g. divider (struct clk_divider
> > > > > *divider) for IMX6Q clock, but this would change the original
> > > > > structure of the CCF code.
> > >
> > > The original Linux's CCF code for iMX relies on using kmalloc
> > > internally:
> > >
> > > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/imx/clk-gate2.c#L139
> > > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk-divider.c#L471
> > >
> > > By using driver_data I've avoided the need to make more changes to
> > > the original Linux code.
> > >
> > > I could use udevice's priv with automatic data allocation but then
> > > the CCF ported code would require more changes and considering the
> > > (from the outset) need to "fit" this code into U-Boot's DM, it
> > > drives away from the original Linux code.
> >
> > Is the main change the need to cast driver_data?
>
> The main change would be to remove the per clock device memory
> allocation code (with exit paths) from the original CCF code.
>
> This shall not be so difficult.
>
> > Perhaps that could be
> > hidden in a helper function/macro, so that in U-Boot it can hide the
> > use of (struct clk_uc_priv *)dev_get_uclass_priv(clk->dev))>parent  ?
>
> Helper function would help to some extend as we operate on structures
> similar to:
>
> struct clk_gate2 {
>         struct clk clk;
>         void __iomem    *reg;
>         u8              bit_idx;
>         u8              cgr_val;
>         u8              flags;
> };
>
> The helper would return struct clk's address which is the same as
> struct's clk_gate2 (this is assured by C standard).
>
> >
> > >
> > >
> > > > >
> > > > > The question is if it would be better to use private_alloc_size
> > > > > (and dev->private) or stay with driver_data.
> > > > > The former requires some rewritting in CCF original code (to
> > > > > remove (c)malloc, etc), but comply with u-boot DM. The latter
> > > > > allows re-using the CCF code as is, but introduces some
> > > > > convention special for CCF (I'm not sure thought if dev->priv
> > > > > is NOT another convention as well).
> > > >
> > > > Yes I would like to avoid malloc() calls in drivers and use the
> > > > in-built mechanism.
> > >
> > > I see your point.
> > >
> > > If the community agrees - I can rewrite the code to use such
> > > approach (but issues pointed out in [*] still apply).
> > >
> > > >
> > > > >
> > > > > - I've added the clk_get_parent(), which reads parent's
> > > > > dev->driver_data 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.
> > > > >
> > > > > - For tests I had to "emulate" CCF code structure to test
> > > > > functionality of clk_get_parent_rate() and clk_get_by_id().
> > > > > Those functions will not work properly with "standard" (i.e.
> > > > > non CCF) clock setup(with not set dev->driver_data to struct
> > > > > clk).
> > > >
> > > > Well I think we need a better approach for that anywat.
> > > > driver_data is used for getting something from the DT.
> > >
> > > Maybe the name (driver_data) was a bit misleading then. For CCF it
> > > stores the back pointer to struct clk (as in fact it is a CCF's
> > > "driver data").
> >
> > Well it seems like a hack to me. Perhaps there is a good reason for it
> > in Linux?
>
> In Linux there is another approach - namely the struct clk (which is
> the main struct for clock management) has pointer to struct clk_core,
> which has pointer to parent(s).
>
> https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk.c#L43
>
> In the case of U-Boot - the CCF wants to work on struct clk, but the
> _main_ data structure for U-Boot is struct udevice. Hence the need to
> have a back pointer (or force struct clk to have NOT pointer to udevice,
> but the udevice itself - then container_of would then do the trick).

The thing I don't understand is that I assumed there is no 1:1
correspondence from struct clk to struct udevice. I thought that we
could have one clock device which supports lots of clk IDs (e.g.
0-23).

>
> > Or is it just convenience?
>
> As stated above - Linux all necessary information has accessible from
> struct clk.

Sure, but we can always find the udevice from the clk.

If we require that clk == udevice then we can go back the other way
too, by using uclass-private data attached to each device.

>
> >
> > >
> > >
> > >
> > > NOTE:
> > >
> > > [*] - I do have a hard time to understand how struct clk shall work
> > > with struct udevice?
> > >
> > > In Linux or Barebox the struct clk is the "main" structure to hold
> > > the clock management data (like freq, ops, flags, parent/sibling
> > > relation, etc).
> >
> > Yes U-Boot has a uniform struct udevice for every device and struct
> > uclass for every class.
> >
> > But the interesting thing here is that clocks have their own
> > parent/sibling relationships, quite independent from the device tree.
>
> But there would be no harm if we could re-use udevice for it. In the
> current CCF (v4) patch set each clk IP block (like mux or gate) is
> modelled as udevice:
>
> https://pastebin.com/uVmwv5FT

I don't see how you can do this...doesn't it mean changing the parents
of existing devices? E.g. if a SPI clock can come from one of 4
parents, do you need to changes its parent in the driver-model tree?

>
> >
> > >
> > > A side observation - we now have three different implementations of
> > > struct clk in U-Boot :-) (two of which have *ops inside :-) )
> >
> > Oh dear.
> >
> > The broadcom iMX ones needs to be converted.
> >
> > >
> > > In the case of U-Boot's DM (./include/clk.h) it only has a
> > > _pointer_ to udevice (which means that I cannot get the struct clk
> > > easily from udevice with container_of()). The struct udevice has
> > > instead the *ops and *parent pointer (to another udevice).
> >
> > Yes that's correct. The struct clk is actually a handle to the clock,
> > and includes an ID number.
>
> You mean the ID number of the clock ?

Yes:

struct clk {
struct udevice *dev;
/*
* Written by of_xlate. We assume a single id is enough for now. In the
* future, we might add more fields here.
*/
unsigned long id;
};


>
> >
> > >
> > > Two problems:
> > >
> > > - Linux CCF code uses massively "struct clk" to handle clock
> > > operations (but not udevice)
> >
> > OK.
> >
> > >
> > > - There is no clear idea of how to implement
> > > struct clk *clk_get_parent(struct clk *) in U-Boot.
> >
> > As above, it seems that this might need to be implemented. I don't
> > think the DM parent/child relationships are good enough for clk, since
> > they are not aware of the ID.
> >
> > >
> > > The reason is that we lack clear information about which udevice's
> > > data field shall be used to store the back pointer from udevice to
> > > struct clk.
> > >
> > > Any hints and ideas are more than welcome.
> >
> > I think it would be good to get Stephen Warren's thoughts on this as
> > he made the change to introduce struct clk.
>
> Ok.
>
> >
> > But at present clk_set_parent() is implemented by calling into the
> > driver. The uclass itself does not maintain information about what is
> > a parent of what.
>
> Linux struct clk has easy access to its parent's struct clk. This is
> what is missing in U-Boot's DM.
>
> >
> > Do we *need* to maintain this information in the uclass?
>
> I think that we don't need. It would be enough to modify struct clk to
> has struct udevice embedded in it (as Linux has struct clk_core), not
> the pointer. Then we can use container_of to get clock and re-use
> struct udevice's parent pointer (and maybe UCLASS_CLK list of devices).
>
> >
> > I think it would be prohibitively expensive to separate out each
> > individual clock into a separate device (udevice), but that would
> > work.
>
> This is the approach as I use now in CCF v4:
> https://pastebin.com/uVmwv5FT
>
> It is expensive but logically correct as each mux, gate, pll is the
> other CLK IP block (device).

OK I see. What is the cost? Is it acceptable for a boot loader?

>
> >
> > The only other option I see is to create a sibling list and parent
> > pointer inside struct clk.
>
> This would be the approach similar to Linux kernel approach.
>
> However, I don't know what were original needs of struct clk (as it did
> not have it). Maybe Stephen can shed some light on it?

Hopefully.

>
> >
> > I suspect this will affect power domain also, although we don't have
> > that yet.
>
> iMX8 has some clocks which needs to be always recalculated as they
> depend on power domains which can be disabled.

OK

>
> >
> > Do you think there is a case for building this into DM itself, such
> > that devices can have a list of IDs for each device, each with
> > independent parent/child relationships?
>
> For iMX6Q the ID of the clock is used to get proper clock in drivers
> (or from DTS). All clock's udevices are stored into UCLASS_CLK list.
> With the ID we get proper udevice and from it and via driver_data the
> struct clk, which is then used in the CCF code to operate on clock
> devices (PLL, gate, mux, etc).
>
> I simply re-used the DM facilities (only missing is the back pointer
> from udevice to struct clk).

Well then you can use dev_get_uclass_priv() for that.

Regards,
Simon

Hi Simon,

> Hi Lukasz,
> 
> On Sun, 19 May 2019 at 15:03, Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Simon,
> >  
> > > Hi Lukasz,
> > >
> > > On Sat, 18 May 2019 at 15:28, Lukasz Majewski <lukma@denx.de>
> > > wrote:  
> > > >
> > > > Hi Simon,
> > > >
> > > > This is not the newest patch set version of CCF (v3 vs. v4),
> > > > but the comments/issues apply.
> > > >  
> > > > > Hi Lukasz,
> > > > >
> > > > > On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski <lukma@denx.de>
> > > > > wrote:  
> > > > > >
> > > > > > This patch series brings the files from Linux kernel to
> > > > > > provide clocks support as it is used on the Linux kernel
> > > > > > with common clock framework [CCF] setup.
> > > > > >
> > > > > > This series also fixes several problems with current clocks
> > > > > > and provides sandbox tests for functions addded to
> > > > > > clk-uclass.c file.
> > > > > >
> > > > > > Design decisions/issues:
> > > > > > =========================
> > > > > >
> > > > > > - U-boot's 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() now caches the previously read data (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). The 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.
> > > > > >
> > > > > > - The "central" structure of this patch series is struct
> > > > > > udevice and its driver_data field contains the struct clk
> > > > > > pointer (to the originally created one).
> > > > > >
> > > > > > - Up till now U-boot's driver model's CLK operates on
> > > > > > udevice (main access to clock is by udevice ops)
> > > > > >   In the CCF the access to struct clk (comprising pointer to
> > > > > > *dev) is possible via dev_get_driver_data()
> > > > > >
> > > > > >   Storing back pointer (from udevice to struct clk) as
> > > > > > driver_data is a convention for CCF.  
> > > > >
> > > > > Ick. Why not use uclass-private data to store this, since
> > > > > every UCLASS_CLK device can have a parent.  
> > > >
> > > > The "private_data" field would be also a good place to store the
> > > > back pointer from udevice to struct clk [*]. The problem with
> > > > CCF and udevice's priv pointer is explained just below:
> > > >  
> > > > >  
> > > > > >
> > > > > > - I could use *private_alloc_size to allocate driver's
> > > > > > 'private" structures (dev->priv) for e.g. divider (struct
> > > > > > clk_divider *divider) for IMX6Q clock, but this would
> > > > > > change the original structure of the CCF code.  
> > > >
> > > > The original Linux's CCF code for iMX relies on using kmalloc
> > > > internally:
> > > >
> > > > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/imx/clk-gate2.c#L139
> > > > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk-divider.c#L471
> > > >
> > > > By using driver_data I've avoided the need to make more changes
> > > > to the original Linux code.
> > > >
> > > > I could use udevice's priv with automatic data allocation but
> > > > then the CCF ported code would require more changes and
> > > > considering the (from the outset) need to "fit" this code into
> > > > U-Boot's DM, it drives away from the original Linux code.  
> > >
> > > Is the main change the need to cast driver_data?  
> >
> > The main change would be to remove the per clock device memory
> > allocation code (with exit paths) from the original CCF code.
> >
> > This shall not be so difficult.
> >  
> > > Perhaps that could be
> > > hidden in a helper function/macro, so that in U-Boot it can hide
> > > the use of (struct clk_uc_priv
> > > *)dev_get_uclass_priv(clk->dev))>parent  ?  
> >
> > Helper function would help to some extend as we operate on
> > structures similar to:
> >
> > struct clk_gate2 {
> >         struct clk clk;
> >         void __iomem    *reg;
> >         u8              bit_idx;
> >         u8              cgr_val;
> >         u8              flags;
> > };
> >
> > The helper would return struct clk's address which is the same as
> > struct's clk_gate2 (this is assured by C standard).
> >  
> > >  
> > > >
> > > >  
> > > > > >
> > > > > > The question is if it would be better to use
> > > > > > private_alloc_size (and dev->private) or stay with
> > > > > > driver_data. The former requires some rewritting in CCF
> > > > > > original code (to remove (c)malloc, etc), but comply with
> > > > > > u-boot DM. The latter allows re-using the CCF code as is,
> > > > > > but introduces some convention special for CCF (I'm not
> > > > > > sure thought if dev->priv is NOT another convention as
> > > > > > well).  
> > > > >
> > > > > Yes I would like to avoid malloc() calls in drivers and use
> > > > > the in-built mechanism.  
> > > >
> > > > I see your point.
> > > >
> > > > If the community agrees - I can rewrite the code to use such
> > > > approach (but issues pointed out in [*] still apply).
> > > >  
> > > > >  
> > > > > >
> > > > > > - I've added the clk_get_parent(), which reads parent's
> > > > > > dev->driver_data 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.
> > > > > >
> > > > > > - For tests I had to "emulate" CCF code structure to test
> > > > > > functionality of clk_get_parent_rate() and clk_get_by_id().
> > > > > > Those functions will not work properly with "standard" (i.e.
> > > > > > non CCF) clock setup(with not set dev->driver_data to struct
> > > > > > clk).  
> > > > >
> > > > > Well I think we need a better approach for that anywat.
> > > > > driver_data is used for getting something from the DT.  
> > > >
> > > > Maybe the name (driver_data) was a bit misleading then. For CCF
> > > > it stores the back pointer to struct clk (as in fact it is a
> > > > CCF's "driver data").  
> > >
> > > Well it seems like a hack to me. Perhaps there is a good reason
> > > for it in Linux?  
> >
> > In Linux there is another approach - namely the struct clk (which is
> > the main struct for clock management) has pointer to struct
> > clk_core, which has pointer to parent(s).
> >
> > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk.c#L43
> >
> > In the case of U-Boot - the CCF wants to work on struct clk, but the
> > _main_ data structure for U-Boot is struct udevice. Hence the need
> > to have a back pointer (or force struct clk to have NOT pointer to
> > udevice, but the udevice itself - then container_of would then do
> > the trick).  
> 
> The thing I don't understand is that I assumed there is no 1:1
> correspondence from struct clk to struct udevice.

For CCF there is 1:1 match - i.e. each struct udevice has a matching
struct clk.

> I thought that we
> could have one clock device which supports lots of clk IDs (e.g.
> 0-23).

On IMX CCF the clock ID is a unique number to refer to a clock.

For example 163 is spi0, 164 is spi1, 165 is spi2, etc. The CCF uses
this ID to get proper struct clk.

In case of CCF port in U-Boot we use clk ID to get proper udevice by
searching UCLASS_CLK devices linked together. Then the struct clk is
read with:
struct clk *clk = (struct clk *)dev_get_driver_data(dev);
And we do have match when clk->id == id.


Having one device supporting many IDs would be not compatible with CCF
where each clock has its own instance of device and clock specific
structure.

> 
> >  
> > > Or is it just convenience?  
> >
> > As stated above - Linux all necessary information has accessible
> > from struct clk.  
> 
> Sure, but we can always find the udevice from the clk.

Yes, correct.

However clocks (represented as struct udevices) are linked in a single,
common uclass (UCLASS_CLK).

> 
> If we require that clk == udevice then we can go back the other way
> too, by using uclass-private data attached to each device.

That may work.

The struct udevice's priv would hold clock specific data structure
(like struct clk_gate2  - automatically allocated and released).

And the uclass_priv would contain the pointer to struct clk itself
(however, in CCF it is always the same as clock specific data structure
- being the first field in the structure).

> 
> >  
> > >  
> > > >
> > > >
> > > >
> > > > NOTE:
> > > >
> > > > [*] - I do have a hard time to understand how struct clk shall
> > > > work with struct udevice?
> > > >
> > > > In Linux or Barebox the struct clk is the "main" structure to
> > > > hold the clock management data (like freq, ops, flags,
> > > > parent/sibling relation, etc).  
> > >
> > > Yes U-Boot has a uniform struct udevice for every device and
> > > struct uclass for every class.
> > >
> > > But the interesting thing here is that clocks have their own
> > > parent/sibling relationships, quite independent from the device
> > > tree.  
> >
> > But there would be no harm if we could re-use udevice for it. In the
> > current CCF (v4) patch set each clk IP block (like mux or gate) is
> > modelled as udevice:
> >
> > https://pastebin.com/uVmwv5FT  
> 
> I don't see how you can do this...doesn't it mean changing the parents
> of existing devices? E.g. if a SPI clock can come from one of 4
> parents, do you need to changes its parent in the driver-model tree?

Yes. The hierarchy is build when the "generic" clock driver is probed (@
clk/imx/clk-imx6q.c) by using the udevice's parent pointer.

If afterwards I need to change clock parent, then I simply change
parent pointers in udevices.

> 
> >  
> > >  
> > > >
> > > > A side observation - we now have three different
> > > > implementations of struct clk in U-Boot :-) (two of which have
> > > > *ops inside :-) )  
> > >
> > > Oh dear.
> > >
> > > The broadcom iMX ones needs to be converted.
> > >  
> > > >
> > > > In the case of U-Boot's DM (./include/clk.h) it only has a
> > > > _pointer_ to udevice (which means that I cannot get the struct
> > > > clk easily from udevice with container_of()). The struct
> > > > udevice has instead the *ops and *parent pointer (to another
> > > > udevice).  
> > >
> > > Yes that's correct. The struct clk is actually a handle to the
> > > clock, and includes an ID number.  
> >
> > You mean the ID number of the clock ?  
> 
> Yes:
> 
> struct clk {
> struct udevice *dev;
> /*
> * Written by of_xlate. We assume a single id is enough for now. In the
> * future, we might add more fields here.
> */
> unsigned long id;
> };
> 
> 
> >  
> > >  
> > > >
> > > > Two problems:
> > > >
> > > > - Linux CCF code uses massively "struct clk" to handle clock
> > > > operations (but not udevice)  
> > >
> > > OK.
> > >  
> > > >
> > > > - There is no clear idea of how to implement
> > > > struct clk *clk_get_parent(struct clk *) in U-Boot.  
> > >
> > > As above, it seems that this might need to be implemented. I don't
> > > think the DM parent/child relationships are good enough for clk,
> > > since they are not aware of the ID.
> > >  
> > > >
> > > > The reason is that we lack clear information about which
> > > > udevice's data field shall be used to store the back pointer
> > > > from udevice to struct clk.
> > > >
> > > > Any hints and ideas are more than welcome.  
> > >
> > > I think it would be good to get Stephen Warren's thoughts on this
> > > as he made the change to introduce struct clk.  
> >
> > Ok.
> >  
> > >
> > > But at present clk_set_parent() is implemented by calling into the
> > > driver. The uclass itself does not maintain information about
> > > what is a parent of what.  
> >
> > Linux struct clk has easy access to its parent's struct clk. This is
> > what is missing in U-Boot's DM.
> >  
> > >
> > > Do we *need* to maintain this information in the uclass?  
> >
> > I think that we don't need. It would be enough to modify struct clk
> > to has struct udevice embedded in it (as Linux has struct
> > clk_core), not the pointer. Then we can use container_of to get
> > clock and re-use struct udevice's parent pointer (and maybe
> > UCLASS_CLK list of devices). 
> > >
> > > I think it would be prohibitively expensive to separate out each
> > > individual clock into a separate device (udevice), but that would
> > > work.  
> >
> > This is the approach as I use now in CCF v4:
> > https://pastebin.com/uVmwv5FT
> >
> > It is expensive but logically correct as each mux, gate, pll is the
> > other CLK IP block (device).  
> 
> OK I see. What is the cost? Is it acceptable for a boot loader?

To make it really small we would need to use OF_PLATDATA.

For e.g. iMX6Q - I'm able to boot with this CCF port running in both
SPL and U-Boot proper.

But, yes the cost is to have the larger binary as we do have larger
section with udevices linker list.

Original Linux CCF code uses ~1KiB dynamic table to store pointers to
struct clk addressed by ID number. In U-Boot instead - I add those
devices to UCLASS_CLK list of devices (as 1KiB is a lot for SPL).

> 
> >  
> > >
> > > The only other option I see is to create a sibling list and parent
> > > pointer inside struct clk.  
> >
> > This would be the approach similar to Linux kernel approach.
> >
> > However, I don't know what were original needs of struct clk (as it
> > did not have it). Maybe Stephen can shed some light on it?  
> 
> Hopefully.
> 
> >  
> > >
> > > I suspect this will affect power domain also, although we don't
> > > have that yet.  
> >
> > iMX8 has some clocks which needs to be always recalculated as they
> > depend on power domains which can be disabled.  
> 
> OK
> 
> >  
> > >
> > > Do you think there is a case for building this into DM itself,
> > > such that devices can have a list of IDs for each device, each
> > > with independent parent/child relationships?  
> >
> > For iMX6Q the ID of the clock is used to get proper clock in drivers
> > (or from DTS). All clock's udevices are stored into UCLASS_CLK list.
> > With the ID we get proper udevice and from it and via driver_data
> > the struct clk, which is then used in the CCF code to operate on
> > clock devices (PLL, gate, mux, etc).
> >
> > I simply re-used the DM facilities (only missing is the back pointer
> > from udevice to struct clk).  
> 
> Well then you can use dev_get_uclass_priv() for that.

I think that it might be enough to use udevice's priv as clock specific
structure (struct clk_gate2) has the struct clock embedded in it.

> 
> Regards,
> Simon




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@denx.de

Hi Lukasz,

On Tue, 21 May 2019 at 08:48, Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Simon,
>
> > Hi Lukasz,
> >
> > On Sun, 19 May 2019 at 15:03, Lukasz Majewski <lukma@denx.de> wrote:
> > >
> > > Hi Simon,
> > >
> > > > Hi Lukasz,
> > > >
> > > > On Sat, 18 May 2019 at 15:28, Lukasz Majewski <lukma@denx.de>
> > > > wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > This is not the newest patch set version of CCF (v3 vs. v4),
> > > > > but the comments/issues apply.
> > > > >
> > > > > > Hi Lukasz,
> > > > > >
> > > > > > On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski <lukma@denx.de>
> > > > > > wrote:
> > > > > > >
> > > > > > > This patch series brings the files from Linux kernel to
> > > > > > > provide clocks support as it is used on the Linux kernel
> > > > > > > with common clock framework [CCF] setup.
> > > > > > >
> > > > > > > This series also fixes several problems with current clocks
> > > > > > > and provides sandbox tests for functions addded to
> > > > > > > clk-uclass.c file.
> > > > > > >
> > > > > > > Design decisions/issues:
> > > > > > > =========================
> > > > > > >
> > > > > > > - U-boot's 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() now caches the previously read data (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). The 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.
> > > > > > >
> > > > > > > - The "central" structure of this patch series is struct
> > > > > > > udevice and its driver_data field contains the struct clk
> > > > > > > pointer (to the originally created one).
> > > > > > >
> > > > > > > - Up till now U-boot's driver model's CLK operates on
> > > > > > > udevice (main access to clock is by udevice ops)
> > > > > > >   In the CCF the access to struct clk (comprising pointer to
> > > > > > > *dev) is possible via dev_get_driver_data()
> > > > > > >
> > > > > > >   Storing back pointer (from udevice to struct clk) as
> > > > > > > driver_data is a convention for CCF.
> > > > > >
> > > > > > Ick. Why not use uclass-private data to store this, since
> > > > > > every UCLASS_CLK device can have a parent.
> > > > >
> > > > > The "private_data" field would be also a good place to store the
> > > > > back pointer from udevice to struct clk [*]. The problem with
> > > > > CCF and udevice's priv pointer is explained just below:
> > > > >
> > > > > >
> > > > > > >
> > > > > > > - I could use *private_alloc_size to allocate driver's
> > > > > > > 'private" structures (dev->priv) for e.g. divider (struct
> > > > > > > clk_divider *divider) for IMX6Q clock, but this would
> > > > > > > change the original structure of the CCF code.
> > > > >
> > > > > The original Linux's CCF code for iMX relies on using kmalloc
> > > > > internally:
> > > > >
> > > > > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/imx/clk-gate2.c#L139
> > > > > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk-divider.c#L471
> > > > >
> > > > > By using driver_data I've avoided the need to make more changes
> > > > > to the original Linux code.
> > > > >
> > > > > I could use udevice's priv with automatic data allocation but
> > > > > then the CCF ported code would require more changes and
> > > > > considering the (from the outset) need to "fit" this code into
> > > > > U-Boot's DM, it drives away from the original Linux code.
> > > >
> > > > Is the main change the need to cast driver_data?
> > >
> > > The main change would be to remove the per clock device memory
> > > allocation code (with exit paths) from the original CCF code.
> > >
> > > This shall not be so difficult.
> > >
> > > > Perhaps that could be
> > > > hidden in a helper function/macro, so that in U-Boot it can hide
> > > > the use of (struct clk_uc_priv
> > > > *)dev_get_uclass_priv(clk->dev))>parent  ?
> > >
> > > Helper function would help to some extend as we operate on
> > > structures similar to:
> > >
> > > struct clk_gate2 {
> > >         struct clk clk;
> > >         void __iomem    *reg;
> > >         u8              bit_idx;
> > >         u8              cgr_val;
> > >         u8              flags;
> > > };
> > >
> > > The helper would return struct clk's address which is the same as
> > > struct's clk_gate2 (this is assured by C standard).
> > >
> > > >
> > > > >
> > > > >
> > > > > > >
> > > > > > > The question is if it would be better to use
> > > > > > > private_alloc_size (and dev->private) or stay with
> > > > > > > driver_data. The former requires some rewritting in CCF
> > > > > > > original code (to remove (c)malloc, etc), but comply with
> > > > > > > u-boot DM. The latter allows re-using the CCF code as is,
> > > > > > > but introduces some convention special for CCF (I'm not
> > > > > > > sure thought if dev->priv is NOT another convention as
> > > > > > > well).
> > > > > >
> > > > > > Yes I would like to avoid malloc() calls in drivers and use
> > > > > > the in-built mechanism.
> > > > >
> > > > > I see your point.
> > > > >
> > > > > If the community agrees - I can rewrite the code to use such
> > > > > approach (but issues pointed out in [*] still apply).
> > > > >
> > > > > >
> > > > > > >
> > > > > > > - I've added the clk_get_parent(), which reads parent's
> > > > > > > dev->driver_data 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.
> > > > > > >
> > > > > > > - For tests I had to "emulate" CCF code structure to test
> > > > > > > functionality of clk_get_parent_rate() and clk_get_by_id().
> > > > > > > Those functions will not work properly with "standard" (i.e.
> > > > > > > non CCF) clock setup(with not set dev->driver_data to struct
> > > > > > > clk).
> > > > > >
> > > > > > Well I think we need a better approach for that anywat.
> > > > > > driver_data is used for getting something from the DT.
> > > > >
> > > > > Maybe the name (driver_data) was a bit misleading then. For CCF
> > > > > it stores the back pointer to struct clk (as in fact it is a
> > > > > CCF's "driver data").
> > > >
> > > > Well it seems like a hack to me. Perhaps there is a good reason
> > > > for it in Linux?
> > >
> > > In Linux there is another approach - namely the struct clk (which is
> > > the main struct for clock management) has pointer to struct
> > > clk_core, which has pointer to parent(s).
> > >
> > > https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk.c#L43
> > >
> > > In the case of U-Boot - the CCF wants to work on struct clk, but the
> > > _main_ data structure for U-Boot is struct udevice. Hence the need
> > > to have a back pointer (or force struct clk to have NOT pointer to
> > > udevice, but the udevice itself - then container_of would then do
> > > the trick).
> >
> > The thing I don't understand is that I assumed there is no 1:1
> > correspondence from struct clk to struct udevice.
>
> For CCF there is 1:1 match - i.e. each struct udevice has a matching
> struct clk.
>
> > I thought that we
> > could have one clock device which supports lots of clk IDs (e.g.
> > 0-23).
>
> On IMX CCF the clock ID is a unique number to refer to a clock.
>
> For example 163 is spi0, 164 is spi1, 165 is spi2, etc. The CCF uses
> this ID to get proper struct clk.
>
> In case of CCF port in U-Boot we use clk ID to get proper udevice by
> searching UCLASS_CLK devices linked together. Then the struct clk is
> read with:
> struct clk *clk = (struct clk *)dev_get_driver_data(dev);
> And we do have match when clk->id == id.
>
>
> Having one device supporting many IDs would be not compatible with CCF
> where each clock has its own instance of device and clock specific
> structure.

OK good.

>
> >
> > >
> > > > Or is it just convenience?
> > >
> > > As stated above - Linux all necessary information has accessible
> > > from struct clk.
> >
> > Sure, but we can always find the udevice from the clk.
>
> Yes, correct.
>
> However clocks (represented as struct udevices) are linked in a single,
> common uclass (UCLASS_CLK).

OK good.

>
> >
> > If we require that clk == udevice then we can go back the other way
> > too, by using uclass-private data attached to each device.
>
> That may work.
>
> The struct udevice's priv would hold clock specific data structure
> (like struct clk_gate2  - automatically allocated and released).
>
> And the uclass_priv would contain the pointer to struct clk itself
> (however, in CCF it is always the same as clock specific data structure
> - being the first field in the structure).

Yes, or struct clk_uc_priv could contain a *member* which is a pointer
to struct clk.

>
> >
> > >
> > > >
> > > > >
> > > > >
> > > > >
> > > > > NOTE:
> > > > >
> > > > > [*] - I do have a hard time to understand how struct clk shall
> > > > > work with struct udevice?
> > > > >
> > > > > In Linux or Barebox the struct clk is the "main" structure to
> > > > > hold the clock management data (like freq, ops, flags,
> > > > > parent/sibling relation, etc).
> > > >
> > > > Yes U-Boot has a uniform struct udevice for every device and
> > > > struct uclass for every class.
> > > >
> > > > But the interesting thing here is that clocks have their own
> > > > parent/sibling relationships, quite independent from the device
> > > > tree.
> > >
> > > But there would be no harm if we could re-use udevice for it. In the
> > > current CCF (v4) patch set each clk IP block (like mux or gate) is
> > > modelled as udevice:
> > >
> > > https://pastebin.com/uVmwv5FT
> >
> > I don't see how you can do this...doesn't it mean changing the parents
> > of existing devices? E.g. if a SPI clock can come from one of 4
> > parents, do you need to changes its parent in the driver-model tree?
>
> Yes. The hierarchy is build when the "generic" clock driver is probed (@
> clk/imx/clk-imx6q.c) by using the udevice's parent pointer.
>
> If afterwards I need to change clock parent, then I simply change
> parent pointers in udevices.

Well if you do that, you need to update the sibling lists
(device->sibling_node).

>
> >
> > >
> > > >
> > > > >
> > > > > A side observation - we now have three different
> > > > > implementations of struct clk in U-Boot :-) (two of which have
> > > > > *ops inside :-) )
> > > >
> > > > Oh dear.
> > > >
> > > > The broadcom iMX ones needs to be converted.
> > > >
> > > > >
> > > > > In the case of U-Boot's DM (./include/clk.h) it only has a
> > > > > _pointer_ to udevice (which means that I cannot get the struct
> > > > > clk easily from udevice with container_of()). The struct
> > > > > udevice has instead the *ops and *parent pointer (to another
> > > > > udevice).
> > > >
> > > > Yes that's correct. The struct clk is actually a handle to the
> > > > clock, and includes an ID number.
> > >
> > > You mean the ID number of the clock ?
> >
> > Yes:
> >
> > struct clk {
> > struct udevice *dev;
> > /*
> > * Written by of_xlate. We assume a single id is enough for now. In the
> > * future, we might add more fields here.
> > */
> > unsigned long id;
> > };
> >
> >
> > >
> > > >
> > > > >
> > > > > Two problems:
> > > > >
> > > > > - Linux CCF code uses massively "struct clk" to handle clock
> > > > > operations (but not udevice)
> > > >
> > > > OK.
> > > >
> > > > >
> > > > > - There is no clear idea of how to implement
> > > > > struct clk *clk_get_parent(struct clk *) in U-Boot.
> > > >
> > > > As above, it seems that this might need to be implemented. I don't
> > > > think the DM parent/child relationships are good enough for clk,
> > > > since they are not aware of the ID.
> > > >
> > > > >
> > > > > The reason is that we lack clear information about which
> > > > > udevice's data field shall be used to store the back pointer
> > > > > from udevice to struct clk.
> > > > >
> > > > > Any hints and ideas are more than welcome.
> > > >
> > > > I think it would be good to get Stephen Warren's thoughts on this
> > > > as he made the change to introduce struct clk.
> > >
> > > Ok.
> > >
> > > >
> > > > But at present clk_set_parent() is implemented by calling into the
> > > > driver. The uclass itself does not maintain information about
> > > > what is a parent of what.
> > >
> > > Linux struct clk has easy access to its parent's struct clk. This is
> > > what is missing in U-Boot's DM.
> > >
> > > >
> > > > Do we *need* to maintain this information in the uclass?
> > >
> > > I think that we don't need. It would be enough to modify struct clk
> > > to has struct udevice embedded in it (as Linux has struct
> > > clk_core), not the pointer. Then we can use container_of to get
> > > clock and re-use struct udevice's parent pointer (and maybe
> > > UCLASS_CLK list of devices).
> > > >
> > > > I think it would be prohibitively expensive to separate out each
> > > > individual clock into a separate device (udevice), but that would
> > > > work.
> > >
> > > This is the approach as I use now in CCF v4:
> > > https://pastebin.com/uVmwv5FT
> > >
> > > It is expensive but logically correct as each mux, gate, pll is the
> > > other CLK IP block (device).
> >
> > OK I see. What is the cost? Is it acceptable for a boot loader?
>
> To make it really small we would need to use OF_PLATDATA.
>
> For e.g. iMX6Q - I'm able to boot with this CCF port running in both
> SPL and U-Boot proper.
>
> But, yes the cost is to have the larger binary as we do have larger
> section with udevices linker list.
>
> Original Linux CCF code uses ~1KiB dynamic table to store pointers to
> struct clk addressed by ID number. In U-Boot instead - I add those
> devices to UCLASS_CLK list of devices (as 1KiB is a lot for SPL).

OK.

>
> >
> > >
> > > >
> > > > The only other option I see is to create a sibling list and parent
> > > > pointer inside struct clk.
> > >
> > > This would be the approach similar to Linux kernel approach.
> > >
> > > However, I don't know what were original needs of struct clk (as it
> > > did not have it). Maybe Stephen can shed some light on it?
> >
> > Hopefully.
> >
> > >
> > > >
> > > > I suspect this will affect power domain also, although we don't
> > > > have that yet.
> > >
> > > iMX8 has some clocks which needs to be always recalculated as they
> > > depend on power domains which can be disabled.
> >
> > OK
> >
> > >
> > > >
> > > > Do you think there is a case for building this into DM itself,
> > > > such that devices can have a list of IDs for each device, each
> > > > with independent parent/child relationships?
> > >
> > > For iMX6Q the ID of the clock is used to get proper clock in drivers
> > > (or from DTS). All clock's udevices are stored into UCLASS_CLK list.
> > > With the ID we get proper udevice and from it and via driver_data
> > > the struct clk, which is then used in the CCF code to operate on
> > > clock devices (PLL, gate, mux, etc).
> > >
> > > I simply re-used the DM facilities (only missing is the back pointer
> > > from udevice to struct clk).
> >
> > Well then you can use dev_get_uclass_priv() for that.
>
> I think that it might be enough to use udevice's priv as clock specific
> structure (struct clk_gate2) has the struct clock embedded in it.

Why do that? The uclass is specifically designed to hold data that is
common to the uclass.

Regards,
Simon

Hi All

Will we force to use CCF in future?
And Is there possibility this feature would land in v2019.07? 
If not, I'll try to convert i.MX8MM CLK to no-CCF DM CLK version, since
i.MX8MM has been pending for long time.

Thanks,
Peng.
> Subject: Re: [PATCH v3 00/11] clk: Port Linux common clock framework [CCF]
> to U-boot (tag: 5.0-rc3)
> 
> Hi Lukasz,
> 
> On Tue, 21 May 2019 at 08:48, Lukasz Majewski <lukma@denx.de> wrote:
> >
> > Hi Simon,
> >
> > > Hi Lukasz,
> > >
> > > On Sun, 19 May 2019 at 15:03, Lukasz Majewski <lukma@denx.de>
> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > > Hi Lukasz,
> > > > >
> > > > > On Sat, 18 May 2019 at 15:28, Lukasz Majewski <lukma@denx.de>
> > > > > wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > This is not the newest patch set version of CCF (v3 vs. v4),
> > > > > > but the comments/issues apply.
> > > > > >
> > > > > > > Hi Lukasz,
> > > > > > >
> > > > > > > On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski
> > > > > > > <lukma@denx.de>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > This patch series brings the files from Linux kernel to
> > > > > > > > provide clocks support as it is used on the Linux kernel
> > > > > > > > with common clock framework [CCF] setup.
> > > > > > > >
> > > > > > > > This series also fixes several problems with current
> > > > > > > > clocks and provides sandbox tests for functions addded to
> > > > > > > > clk-uclass.c file.
> > > > > > > >
> > > > > > > > Design decisions/issues:
> > > > > > > > =========================
> > > > > > > >
> > > > > > > > - U-boot's 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() now caches the previously read data
> > > > > > > > (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). The
> > > > > > > > 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.
> > > > > > > >
> > > > > > > > - The "central" structure of this patch series is struct
> > > > > > > > udevice and its driver_data field contains the struct clk
> > > > > > > > pointer (to the originally created one).
> > > > > > > >
> > > > > > > > - Up till now U-boot's driver model's CLK operates on
> > > > > > > > udevice (main access to clock is by udevice ops)
> > > > > > > >   In the CCF the access to struct clk (comprising pointer
> > > > > > > > to
> > > > > > > > *dev) is possible via dev_get_driver_data()
> > > > > > > >
> > > > > > > >   Storing back pointer (from udevice to struct clk) as
> > > > > > > > driver_data is a convention for CCF.
> > > > > > >
> > > > > > > Ick. Why not use uclass-private data to store this, since
> > > > > > > every UCLASS_CLK device can have a parent.
> > > > > >
> > > > > > The "private_data" field would be also a good place to store
> > > > > > the back pointer from udevice to struct clk [*]. The problem
> > > > > > with CCF and udevice's priv pointer is explained just below:
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > - I could use *private_alloc_size to allocate driver's
> > > > > > > > 'private" structures (dev->priv) for e.g. divider (struct
> > > > > > > > clk_divider *divider) for IMX6Q clock, but this would
> > > > > > > > change the original structure of the CCF code.
> > > > > >
> > > > > > The original Linux's CCF code for iMX relies on using kmalloc
> > > > > > internally:
> > > > > >
> > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2
> > > > > > F%2Felixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fc
> > > > > >
> lk%2Fimx%2Fclk-gate2.c%23L139&amp;data=02%7C01%7Cpeng.fan%40nx
> > > > > >
> p.com%7C995c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92
> > > > > >
> cd99c5c301635%7C0%7C0%7C636940832249605030&amp;sdata=NJdvX7JfV
> > > > > >
> g2Qd8H%2FEmnzeNS1v5xhz4AaMRhSUpo7MxI%3D&amp;reserved=0
> > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2
> > > > > > F%2Felixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fc
> > > > > >
> lk%2Fclk-divider.c%23L471&amp;data=02%7C01%7Cpeng.fan%40nxp.co
> > > > > >
> m%7C995c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99
> > > > > >
> c5c301635%7C0%7C0%7C636940832249605030&amp;sdata=iF66y5IKaQY69
> > > > > > vyFV%2BY5HSsZiKDb4s%2BN2yMPOBNDOqA%3D&amp;reserved=0
> > > > > >
> > > > > > By using driver_data I've avoided the need to make more
> > > > > > changes to the original Linux code.
> > > > > >
> > > > > > I could use udevice's priv with automatic data allocation but
> > > > > > then the CCF ported code would require more changes and
> > > > > > considering the (from the outset) need to "fit" this code into
> > > > > > U-Boot's DM, it drives away from the original Linux code.
> > > > >
> > > > > Is the main change the need to cast driver_data?
> > > >
> > > > The main change would be to remove the per clock device memory
> > > > allocation code (with exit paths) from the original CCF code.
> > > >
> > > > This shall not be so difficult.
> > > >
> > > > > Perhaps that could be
> > > > > hidden in a helper function/macro, so that in U-Boot it can hide
> > > > > the use of (struct clk_uc_priv
> > > > > *)dev_get_uclass_priv(clk->dev))>parent  ?
> > > >
> > > > Helper function would help to some extend as we operate on
> > > > structures similar to:
> > > >
> > > > struct clk_gate2 {
> > > >         struct clk clk;
> > > >         void __iomem    *reg;
> > > >         u8              bit_idx;
> > > >         u8              cgr_val;
> > > >         u8              flags;
> > > > };
> > > >
> > > > The helper would return struct clk's address which is the same as
> > > > struct's clk_gate2 (this is assured by C standard).
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > > >
> > > > > > > > The question is if it would be better to use
> > > > > > > > private_alloc_size (and dev->private) or stay with
> > > > > > > > driver_data. The former requires some rewritting in CCF
> > > > > > > > original code (to remove (c)malloc, etc), but comply with
> > > > > > > > u-boot DM. The latter allows re-using the CCF code as is,
> > > > > > > > but introduces some convention special for CCF (I'm not
> > > > > > > > sure thought if dev->priv is NOT another convention as
> > > > > > > > well).
> > > > > > >
> > > > > > > Yes I would like to avoid malloc() calls in drivers and use
> > > > > > > the in-built mechanism.
> > > > > >
> > > > > > I see your point.
> > > > > >
> > > > > > If the community agrees - I can rewrite the code to use such
> > > > > > approach (but issues pointed out in [*] still apply).
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > - I've added the clk_get_parent(), which reads parent's
> > > > > > > > dev->driver_data 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.
> > > > > > > >
> > > > > > > > - For tests I had to "emulate" CCF code structure to test
> > > > > > > > functionality of clk_get_parent_rate() and clk_get_by_id().
> > > > > > > > Those functions will not work properly with "standard" (i.e.
> > > > > > > > non CCF) clock setup(with not set dev->driver_data to
> > > > > > > > struct clk).
> > > > > > >
> > > > > > > Well I think we need a better approach for that anywat.
> > > > > > > driver_data is used for getting something from the DT.
> > > > > >
> > > > > > Maybe the name (driver_data) was a bit misleading then. For
> > > > > > CCF it stores the back pointer to struct clk (as in fact it is
> > > > > > a CCF's "driver data").
> > > > >
> > > > > Well it seems like a hack to me. Perhaps there is a good reason
> > > > > for it in Linux?
> > > >
> > > > In Linux there is another approach - namely the struct clk (which
> > > > is the main struct for clock management) has pointer to struct
> > > > clk_core, which has pointer to parent(s).
> > > >
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > elixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fclk%2Fclk
> > > > .c%23L43&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7C995c4869
> e12e471bd
> > > >
> b5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> 63694
> > > >
> 0832249605030&amp;sdata=63VsTs6JusNw%2FT4mBpfsFLUZKXtyTpZlYx99jy
> 36
> > > > 6Mk%3D&amp;reserved=0
> > > >
> > > > In the case of U-Boot - the CCF wants to work on struct clk, but
> > > > the _main_ data structure for U-Boot is struct udevice. Hence the
> > > > need to have a back pointer (or force struct clk to have NOT
> > > > pointer to udevice, but the udevice itself - then container_of
> > > > would then do the trick).
> > >
> > > The thing I don't understand is that I assumed there is no 1:1
> > > correspondence from struct clk to struct udevice.
> >
> > For CCF there is 1:1 match - i.e. each struct udevice has a matching
> > struct clk.
> >
> > > I thought that we
> > > could have one clock device which supports lots of clk IDs (e.g.
> > > 0-23).
> >
> > On IMX CCF the clock ID is a unique number to refer to a clock.
> >
> > For example 163 is spi0, 164 is spi1, 165 is spi2, etc. The CCF uses
> > this ID to get proper struct clk.
> >
> > In case of CCF port in U-Boot we use clk ID to get proper udevice by
> > searching UCLASS_CLK devices linked together. Then the struct clk is
> > read with:
> > struct clk *clk = (struct clk *)dev_get_driver_data(dev); And we do
> > have match when clk->id == id.
> >
> >
> > Having one device supporting many IDs would be not compatible with CCF
> > where each clock has its own instance of device and clock specific
> > structure.
> 
> OK good.
> 
> >
> > >
> > > >
> > > > > Or is it just convenience?
> > > >
> > > > As stated above - Linux all necessary information has accessible
> > > > from struct clk.
> > >
> > > Sure, but we can always find the udevice from the clk.
> >
> > Yes, correct.
> >
> > However clocks (represented as struct udevices) are linked in a
> > single, common uclass (UCLASS_CLK).
> 
> OK good.
> 
> >
> > >
> > > If we require that clk == udevice then we can go back the other way
> > > too, by using uclass-private data attached to each device.
> >
> > That may work.
> >
> > The struct udevice's priv would hold clock specific data structure
> > (like struct clk_gate2  - automatically allocated and released).
> >
> > And the uclass_priv would contain the pointer to struct clk itself
> > (however, in CCF it is always the same as clock specific data
> > structure
> > - being the first field in the structure).
> 
> Yes, or struct clk_uc_priv could contain a *member* which is a pointer to
> struct clk.
> 
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > NOTE:
> > > > > >
> > > > > > [*] - I do have a hard time to understand how struct clk shall
> > > > > > work with struct udevice?
> > > > > >
> > > > > > In Linux or Barebox the struct clk is the "main" structure to
> > > > > > hold the clock management data (like freq, ops, flags,
> > > > > > parent/sibling relation, etc).
> > > > >
> > > > > Yes U-Boot has a uniform struct udevice for every device and
> > > > > struct uclass for every class.
> > > > >
> > > > > But the interesting thing here is that clocks have their own
> > > > > parent/sibling relationships, quite independent from the device
> > > > > tree.
> > > >
> > > > But there would be no harm if we could re-use udevice for it. In
> > > > the current CCF (v4) patch set each clk IP block (like mux or
> > > > gate) is modelled as udevice:
> > > >
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > >
> pastebin.com%2FuVmwv5FT&amp;data=02%7C01%7Cpeng.fan%40nxp.com
> %7C99
> > > >
> 5c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%
> > > >
> 7C0%7C0%7C636940832249605030&amp;sdata=XXjDjW9UBtSyuVpNZPeVzY
> AVCrz
> > > > fcnMokz46tqQ1QEo%3D&amp;reserved=0
> > >
> > > I don't see how you can do this...doesn't it mean changing the
> > > parents of existing devices? E.g. if a SPI clock can come from one
> > > of 4 parents, do you need to changes its parent in the driver-model tree?
> >
> > Yes. The hierarchy is build when the "generic" clock driver is probed
> > (@
> > clk/imx/clk-imx6q.c) by using the udevice's parent pointer.
> >
> > If afterwards I need to change clock parent, then I simply change
> > parent pointers in udevices.
> 
> Well if you do that, you need to update the sibling lists (device->sibling_node).
> 
> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > A side observation - we now have three different
> > > > > > implementations of struct clk in U-Boot :-) (two of which have
> > > > > > *ops inside :-) )
> > > > >
> > > > > Oh dear.
> > > > >
> > > > > The broadcom iMX ones needs to be converted.
> > > > >
> > > > > >
> > > > > > In the case of U-Boot's DM (./include/clk.h) it only has a
> > > > > > _pointer_ to udevice (which means that I cannot get the struct
> > > > > > clk easily from udevice with container_of()). The struct
> > > > > > udevice has instead the *ops and *parent pointer (to another
> > > > > > udevice).
> > > > >
> > > > > Yes that's correct. The struct clk is actually a handle to the
> > > > > clock, and includes an ID number.
> > > >
> > > > You mean the ID number of the clock ?
> > >
> > > Yes:
> > >
> > > struct clk {
> > > struct udevice *dev;
> > > /*
> > > * Written by of_xlate. We assume a single id is enough for now. In
> > > the
> > > * future, we might add more fields here.
> > > */
> > > unsigned long id;
> > > };
> > >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > Two problems:
> > > > > >
> > > > > > - Linux CCF code uses massively "struct clk" to handle clock
> > > > > > operations (but not udevice)
> > > > >
> > > > > OK.
> > > > >
> > > > > >
> > > > > > - There is no clear idea of how to implement struct clk
> > > > > > *clk_get_parent(struct clk *) in U-Boot.
> > > > >
> > > > > As above, it seems that this might need to be implemented. I
> > > > > don't think the DM parent/child relationships are good enough
> > > > > for clk, since they are not aware of the ID.
> > > > >
> > > > > >
> > > > > > The reason is that we lack clear information about which
> > > > > > udevice's data field shall be used to store the back pointer
> > > > > > from udevice to struct clk.
> > > > > >
> > > > > > Any hints and ideas are more than welcome.
> > > > >
> > > > > I think it would be good to get Stephen Warren's thoughts on
> > > > > this as he made the change to introduce struct clk.
> > > >
> > > > Ok.
> > > >
> > > > >
> > > > > But at present clk_set_parent() is implemented by calling into
> > > > > the driver. The uclass itself does not maintain information
> > > > > about what is a parent of what.
> > > >
> > > > Linux struct clk has easy access to its parent's struct clk. This
> > > > is what is missing in U-Boot's DM.
> > > >
> > > > >
> > > > > Do we *need* to maintain this information in the uclass?
> > > >
> > > > I think that we don't need. It would be enough to modify struct
> > > > clk to has struct udevice embedded in it (as Linux has struct
> > > > clk_core), not the pointer. Then we can use container_of to get
> > > > clock and re-use struct udevice's parent pointer (and maybe
> > > > UCLASS_CLK list of devices).
> > > > >
> > > > > I think it would be prohibitively expensive to separate out each
> > > > > individual clock into a separate device (udevice), but that
> > > > > would work.
> > > >
> > > > This is the approach as I use now in CCF v4:
> > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > >
> pastebin.com%2FuVmwv5FT&amp;data=02%7C01%7Cpeng.fan%40nxp.com
> %7C99
> > > >
> 5c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c3016
> 35%
> > > >
> 7C0%7C0%7C636940832249605030&amp;sdata=XXjDjW9UBtSyuVpNZPeVzY
> AVCrz
> > > > fcnMokz46tqQ1QEo%3D&amp;reserved=0
> > > >
> > > > It is expensive but logically correct as each mux, gate, pll is
> > > > the other CLK IP block (device).
> > >
> > > OK I see. What is the cost? Is it acceptable for a boot loader?
> >
> > To make it really small we would need to use OF_PLATDATA.
> >
> > For e.g. iMX6Q - I'm able to boot with this CCF port running in both
> > SPL and U-Boot proper.
> >
> > But, yes the cost is to have the larger binary as we do have larger
> > section with udevices linker list.
> >
> > Original Linux CCF code uses ~1KiB dynamic table to store pointers to
> > struct clk addressed by ID number. In U-Boot instead - I add those
> > devices to UCLASS_CLK list of devices (as 1KiB is a lot for SPL).
> 
> OK.
> 
> >
> > >
> > > >
> > > > >
> > > > > The only other option I see is to create a sibling list and
> > > > > parent pointer inside struct clk.
> > > >
> > > > This would be the approach similar to Linux kernel approach.
> > > >
> > > > However, I don't know what were original needs of struct clk (as
> > > > it did not have it). Maybe Stephen can shed some light on it?
> > >
> > > Hopefully.
> > >
> > > >
> > > > >
> > > > > I suspect this will affect power domain also, although we don't
> > > > > have that yet.
> > > >
> > > > iMX8 has some clocks which needs to be always recalculated as they
> > > > depend on power domains which can be disabled.
> > >
> > > OK
> > >
> > > >
> > > > >
> > > > > Do you think there is a case for building this into DM itself,
> > > > > such that devices can have a list of IDs for each device, each
> > > > > with independent parent/child relationships?
> > > >
> > > > For iMX6Q the ID of the clock is used to get proper clock in
> > > > drivers (or from DTS). All clock's udevices are stored into UCLASS_CLK
> list.
> > > > With the ID we get proper udevice and from it and via driver_data
> > > > the struct clk, which is then used in the CCF code to operate on
> > > > clock devices (PLL, gate, mux, etc).
> > > >
> > > > I simply re-used the DM facilities (only missing is the back
> > > > pointer from udevice to struct clk).
> > >
> > > Well then you can use dev_get_uclass_priv() for that.
> >
> > I think that it might be enough to use udevice's priv as clock
> > specific structure (struct clk_gate2) has the struct clock embedded in it.
> 
> Why do that? The uclass is specifically designed to hold data that is common
> to the uclass.
> 
> Regards,
> Simon

On 5/30/19 5:04 AM, Peng Fan wrote:
> Hi All

Hi,

> Will we force to use CCF in future?

Force how ?

> And Is there possibility this feature would land in v2019.07? 

No, it's rc3 already, no way.

> If not, I'll try to convert i.MX8MM CLK to no-CCF DM CLK version, since
> i.MX8MM has been pending for long time.

Good

Also please do not top-post

> Thanks,
> Peng.
>> Subject: Re: [PATCH v3 00/11] clk: Port Linux common clock framework [CCF]
>> to U-boot (tag: 5.0-rc3)
>>
>> Hi Lukasz,
>>
>> On Tue, 21 May 2019 at 08:48, Lukasz Majewski <lukma@denx.de> wrote:
>>>
>>> Hi Simon,
>>>
>>>> Hi Lukasz,
>>>>
>>>> On Sun, 19 May 2019 at 15:03, Lukasz Majewski <lukma@denx.de>
>> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>>> Hi Lukasz,
>>>>>>
>>>>>> On Sat, 18 May 2019 at 15:28, Lukasz Majewski <lukma@denx.de>
>>>>>> wrote:
>>>>>>>
>>>>>>> Hi Simon,
>>>>>>>
>>>>>>> This is not the newest patch set version of CCF (v3 vs. v4),
>>>>>>> but the comments/issues apply.
>>>>>>>
>>>>>>>> Hi Lukasz,
>>>>>>>>
>>>>>>>> On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski
>>>>>>>> <lukma@denx.de>
>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>> This patch series brings the files from Linux kernel to
>>>>>>>>> provide clocks support as it is used on the Linux kernel
>>>>>>>>> with common clock framework [CCF] setup.
>>>>>>>>>
>>>>>>>>> This series also fixes several problems with current
>>>>>>>>> clocks and provides sandbox tests for functions addded to
>>>>>>>>> clk-uclass.c file.
>>>>>>>>>
>>>>>>>>> Design decisions/issues:
>>>>>>>>> =========================
>>>>>>>>>
>>>>>>>>> - U-boot's 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() now caches the previously read data
>>>>>>>>> (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). The
>>>>>>>>> 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.
>>>>>>>>>
>>>>>>>>> - The "central" structure of this patch series is struct
>>>>>>>>> udevice and its driver_data field contains the struct clk
>>>>>>>>> pointer (to the originally created one).
>>>>>>>>>
>>>>>>>>> - Up till now U-boot's driver model's CLK operates on
>>>>>>>>> udevice (main access to clock is by udevice ops)
>>>>>>>>>   In the CCF the access to struct clk (comprising pointer
>>>>>>>>> to
>>>>>>>>> *dev) is possible via dev_get_driver_data()
>>>>>>>>>
>>>>>>>>>   Storing back pointer (from udevice to struct clk) as
>>>>>>>>> driver_data is a convention for CCF.
>>>>>>>>
>>>>>>>> Ick. Why not use uclass-private data to store this, since
>>>>>>>> every UCLASS_CLK device can have a parent.
>>>>>>>
>>>>>>> The "private_data" field would be also a good place to store
>>>>>>> the back pointer from udevice to struct clk [*]. The problem
>>>>>>> with CCF and udevice's priv pointer is explained just below:
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> - I could use *private_alloc_size to allocate driver's
>>>>>>>>> 'private" structures (dev->priv) for e.g. divider (struct
>>>>>>>>> clk_divider *divider) for IMX6Q clock, but this would
>>>>>>>>> change the original structure of the CCF code.
>>>>>>>
>>>>>>> The original Linux's CCF code for iMX relies on using kmalloc
>>>>>>> internally:
>>>>>>>
>>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2
>>>>>>> F%2Felixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fc
>>>>>>>
>> lk%2Fimx%2Fclk-gate2.c%23L139&amp;data=02%7C01%7Cpeng.fan%40nx
>>>>>>>
>> p.com%7C995c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92
>>>>>>>
>> cd99c5c301635%7C0%7C0%7C636940832249605030&amp;sdata=NJdvX7JfV
>>>>>>>
>> g2Qd8H%2FEmnzeNS1v5xhz4AaMRhSUpo7MxI%3D&amp;reserved=0
>>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2
>>>>>>> F%2Felixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fc
>>>>>>>
>> lk%2Fclk-divider.c%23L471&amp;data=02%7C01%7Cpeng.fan%40nxp.co
>>>>>>>
>> m%7C995c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99
>>>>>>>
>> c5c301635%7C0%7C0%7C636940832249605030&amp;sdata=iF66y5IKaQY69
>>>>>>> vyFV%2BY5HSsZiKDb4s%2BN2yMPOBNDOqA%3D&amp;reserved=0
>>>>>>>
>>>>>>> By using driver_data I've avoided the need to make more
>>>>>>> changes to the original Linux code.
>>>>>>>
>>>>>>> I could use udevice's priv with automatic data allocation but
>>>>>>> then the CCF ported code would require more changes and
>>>>>>> considering the (from the outset) need to "fit" this code into
>>>>>>> U-Boot's DM, it drives away from the original Linux code.
>>>>>>
>>>>>> Is the main change the need to cast driver_data?
>>>>>
>>>>> The main change would be to remove the per clock device memory
>>>>> allocation code (with exit paths) from the original CCF code.
>>>>>
>>>>> This shall not be so difficult.
>>>>>
>>>>>> Perhaps that could be
>>>>>> hidden in a helper function/macro, so that in U-Boot it can hide
>>>>>> the use of (struct clk_uc_priv
>>>>>> *)dev_get_uclass_priv(clk->dev))>parent  ?
>>>>>
>>>>> Helper function would help to some extend as we operate on
>>>>> structures similar to:
>>>>>
>>>>> struct clk_gate2 {
>>>>>         struct clk clk;
>>>>>         void __iomem    *reg;
>>>>>         u8              bit_idx;
>>>>>         u8              cgr_val;
>>>>>         u8              flags;
>>>>> };
>>>>>
>>>>> The helper would return struct clk's address which is the same as
>>>>> struct's clk_gate2 (this is assured by C standard).
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> The question is if it would be better to use
>>>>>>>>> private_alloc_size (and dev->private) or stay with
>>>>>>>>> driver_data. The former requires some rewritting in CCF
>>>>>>>>> original code (to remove (c)malloc, etc), but comply with
>>>>>>>>> u-boot DM. The latter allows re-using the CCF code as is,
>>>>>>>>> but introduces some convention special for CCF (I'm not
>>>>>>>>> sure thought if dev->priv is NOT another convention as
>>>>>>>>> well).
>>>>>>>>
>>>>>>>> Yes I would like to avoid malloc() calls in drivers and use
>>>>>>>> the in-built mechanism.
>>>>>>>
>>>>>>> I see your point.
>>>>>>>
>>>>>>> If the community agrees - I can rewrite the code to use such
>>>>>>> approach (but issues pointed out in [*] still apply).
>>>>>>>
>>>>>>>>
>>>>>>>>>
>>>>>>>>> - I've added the clk_get_parent(), which reads parent's
>>>>>>>>> dev->driver_data 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.
>>>>>>>>>
>>>>>>>>> - For tests I had to "emulate" CCF code structure to test
>>>>>>>>> functionality of clk_get_parent_rate() and clk_get_by_id().
>>>>>>>>> Those functions will not work properly with "standard" (i.e.
>>>>>>>>> non CCF) clock setup(with not set dev->driver_data to
>>>>>>>>> struct clk).
>>>>>>>>
>>>>>>>> Well I think we need a better approach for that anywat.
>>>>>>>> driver_data is used for getting something from the DT.
>>>>>>>
>>>>>>> Maybe the name (driver_data) was a bit misleading then. For
>>>>>>> CCF it stores the back pointer to struct clk (as in fact it is
>>>>>>> a CCF's "driver data").
>>>>>>
>>>>>> Well it seems like a hack to me. Perhaps there is a good reason
>>>>>> for it in Linux?
>>>>>
>>>>> In Linux there is another approach - namely the struct clk (which
>>>>> is the main struct for clock management) has pointer to struct
>>>>> clk_core, which has pointer to parent(s).
>>>>>
>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
>>>>> elixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fclk%2Fclk
>>>>> .c%23L43&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7C995c4869
>> e12e471bd
>>>>>
>> b5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
>> 63694
>>>>>
>> 0832249605030&amp;sdata=63VsTs6JusNw%2FT4mBpfsFLUZKXtyTpZlYx99jy
>> 36
>>>>> 6Mk%3D&amp;reserved=0
>>>>>
>>>>> In the case of U-Boot - the CCF wants to work on struct clk, but
>>>>> the _main_ data structure for U-Boot is struct udevice. Hence the
>>>>> need to have a back pointer (or force struct clk to have NOT
>>>>> pointer to udevice, but the udevice itself - then container_of
>>>>> would then do the trick).
>>>>
>>>> The thing I don't understand is that I assumed there is no 1:1
>>>> correspondence from struct clk to struct udevice.
>>>
>>> For CCF there is 1:1 match - i.e. each struct udevice has a matching
>>> struct clk.
>>>
>>>> I thought that we
>>>> could have one clock device which supports lots of clk IDs (e.g.
>>>> 0-23).
>>>
>>> On IMX CCF the clock ID is a unique number to refer to a clock.
>>>
>>> For example 163 is spi0, 164 is spi1, 165 is spi2, etc. The CCF uses
>>> this ID to get proper struct clk.
>>>
>>> In case of CCF port in U-Boot we use clk ID to get proper udevice by
>>> searching UCLASS_CLK devices linked together. Then the struct clk is
>>> read with:
>>> struct clk *clk = (struct clk *)dev_get_driver_data(dev); And we do
>>> have match when clk->id == id.
>>>
>>>
>>> Having one device supporting many IDs would be not compatible with CCF
>>> where each clock has its own instance of device and clock specific
>>> structure.
>>
>> OK good.
>>
>>>
>>>>
>>>>>
>>>>>> Or is it just convenience?
>>>>>
>>>>> As stated above - Linux all necessary information has accessible
>>>>> from struct clk.
>>>>
>>>> Sure, but we can always find the udevice from the clk.
>>>
>>> Yes, correct.
>>>
>>> However clocks (represented as struct udevices) are linked in a
>>> single, common uclass (UCLASS_CLK).
>>
>> OK good.
>>
>>>
>>>>
>>>> If we require that clk == udevice then we can go back the other way
>>>> too, by using uclass-private data attached to each device.
>>>
>>> That may work.
>>>
>>> The struct udevice's priv would hold clock specific data structure
>>> (like struct clk_gate2  - automatically allocated and released).
>>>
>>> And the uclass_priv would contain the pointer to struct clk itself
>>> (however, in CCF it is always the same as clock specific data
>>> structure
>>> - being the first field in the structure).
>>
>> Yes, or struct clk_uc_priv could contain a *member* which is a pointer to
>> struct clk.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> NOTE:
>>>>>>>
>>>>>>> [*] - I do have a hard time to understand how struct clk shall
>>>>>>> work with struct udevice?
>>>>>>>
>>>>>>> In Linux or Barebox the struct clk is the "main" structure to
>>>>>>> hold the clock management data (like freq, ops, flags,
>>>>>>> parent/sibling relation, etc).
>>>>>>
>>>>>> Yes U-Boot has a uniform struct udevice for every device and
>>>>>> struct uclass for every class.
>>>>>>
>>>>>> But the interesting thing here is that clocks have their own
>>>>>> parent/sibling relationships, quite independent from the device
>>>>>> tree.
>>>>>
>>>>> But there would be no harm if we could re-use udevice for it. In
>>>>> the current CCF (v4) patch set each clk IP block (like mux or
>>>>> gate) is modelled as udevice:
>>>>>
>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
>>>>>
>> pastebin.com%2FuVmwv5FT&amp;data=02%7C01%7Cpeng.fan%40nxp.com
>> %7C99
>>>>>
>> 5c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c3016
>> 35%
>>>>>
>> 7C0%7C0%7C636940832249605030&amp;sdata=XXjDjW9UBtSyuVpNZPeVzY
>> AVCrz
>>>>> fcnMokz46tqQ1QEo%3D&amp;reserved=0
>>>>
>>>> I don't see how you can do this...doesn't it mean changing the
>>>> parents of existing devices? E.g. if a SPI clock can come from one
>>>> of 4 parents, do you need to changes its parent in the driver-model tree?
>>>
>>> Yes. The hierarchy is build when the "generic" clock driver is probed
>>> (@
>>> clk/imx/clk-imx6q.c) by using the udevice's parent pointer.
>>>
>>> If afterwards I need to change clock parent, then I simply change
>>> parent pointers in udevices.
>>
>> Well if you do that, you need to update the sibling lists (device->sibling_node).
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> A side observation - we now have three different
>>>>>>> implementations of struct clk in U-Boot :-) (two of which have
>>>>>>> *ops inside :-) )
>>>>>>
>>>>>> Oh dear.
>>>>>>
>>>>>> The broadcom iMX ones needs to be converted.
>>>>>>
>>>>>>>
>>>>>>> In the case of U-Boot's DM (./include/clk.h) it only has a
>>>>>>> _pointer_ to udevice (which means that I cannot get the struct
>>>>>>> clk easily from udevice with container_of()). The struct
>>>>>>> udevice has instead the *ops and *parent pointer (to another
>>>>>>> udevice).
>>>>>>
>>>>>> Yes that's correct. The struct clk is actually a handle to the
>>>>>> clock, and includes an ID number.
>>>>>
>>>>> You mean the ID number of the clock ?
>>>>
>>>> Yes:
>>>>
>>>> struct clk {
>>>> struct udevice *dev;
>>>> /*
>>>> * Written by of_xlate. We assume a single id is enough for now. In
>>>> the
>>>> * future, we might add more fields here.
>>>> */
>>>> unsigned long id;
>>>> };
>>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>> Two problems:
>>>>>>>
>>>>>>> - Linux CCF code uses massively "struct clk" to handle clock
>>>>>>> operations (but not udevice)
>>>>>>
>>>>>> OK.
>>>>>>
>>>>>>>
>>>>>>> - There is no clear idea of how to implement struct clk
>>>>>>> *clk_get_parent(struct clk *) in U-Boot.
>>>>>>
>>>>>> As above, it seems that this might need to be implemented. I
>>>>>> don't think the DM parent/child relationships are good enough
>>>>>> for clk, since they are not aware of the ID.
>>>>>>
>>>>>>>
>>>>>>> The reason is that we lack clear information about which
>>>>>>> udevice's data field shall be used to store the back pointer
>>>>>>> from udevice to struct clk.
>>>>>>>
>>>>>>> Any hints and ideas are more than welcome.
>>>>>>
>>>>>> I think it would be good to get Stephen Warren's thoughts on
>>>>>> this as he made the change to introduce struct clk.
>>>>>
>>>>> Ok.
>>>>>
>>>>>>
>>>>>> But at present clk_set_parent() is implemented by calling into
>>>>>> the driver. The uclass itself does not maintain information
>>>>>> about what is a parent of what.
>>>>>
>>>>> Linux struct clk has easy access to its parent's struct clk. This
>>>>> is what is missing in U-Boot's DM.
>>>>>
>>>>>>
>>>>>> Do we *need* to maintain this information in the uclass?
>>>>>
>>>>> I think that we don't need. It would be enough to modify struct
>>>>> clk to has struct udevice embedded in it (as Linux has struct
>>>>> clk_core), not the pointer. Then we can use container_of to get
>>>>> clock and re-use struct udevice's parent pointer (and maybe
>>>>> UCLASS_CLK list of devices).
>>>>>>
>>>>>> I think it would be prohibitively expensive to separate out each
>>>>>> individual clock into a separate device (udevice), but that
>>>>>> would work.
>>>>>
>>>>> This is the approach as I use now in CCF v4:
>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
>>>>>
>> pastebin.com%2FuVmwv5FT&amp;data=02%7C01%7Cpeng.fan%40nxp.com
>> %7C99
>>>>>
>> 5c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c3016
>> 35%
>>>>>
>> 7C0%7C0%7C636940832249605030&amp;sdata=XXjDjW9UBtSyuVpNZPeVzY
>> AVCrz
>>>>> fcnMokz46tqQ1QEo%3D&amp;reserved=0
>>>>>
>>>>> It is expensive but logically correct as each mux, gate, pll is
>>>>> the other CLK IP block (device).
>>>>
>>>> OK I see. What is the cost? Is it acceptable for a boot loader?
>>>
>>> To make it really small we would need to use OF_PLATDATA.
>>>
>>> For e.g. iMX6Q - I'm able to boot with this CCF port running in both
>>> SPL and U-Boot proper.
>>>
>>> But, yes the cost is to have the larger binary as we do have larger
>>> section with udevices linker list.
>>>
>>> Original Linux CCF code uses ~1KiB dynamic table to store pointers to
>>> struct clk addressed by ID number. In U-Boot instead - I add those
>>> devices to UCLASS_CLK list of devices (as 1KiB is a lot for SPL).
>>
>> OK.
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> The only other option I see is to create a sibling list and
>>>>>> parent pointer inside struct clk.
>>>>>
>>>>> This would be the approach similar to Linux kernel approach.
>>>>>
>>>>> However, I don't know what were original needs of struct clk (as
>>>>> it did not have it). Maybe Stephen can shed some light on it?
>>>>
>>>> Hopefully.
>>>>
>>>>>
>>>>>>
>>>>>> I suspect this will affect power domain also, although we don't
>>>>>> have that yet.
>>>>>
>>>>> iMX8 has some clocks which needs to be always recalculated as they
>>>>> depend on power domains which can be disabled.
>>>>
>>>> OK
>>>>
>>>>>
>>>>>>
>>>>>> Do you think there is a case for building this into DM itself,
>>>>>> such that devices can have a list of IDs for each device, each
>>>>>> with independent parent/child relationships?
>>>>>
>>>>> For iMX6Q the ID of the clock is used to get proper clock in
>>>>> drivers (or from DTS). All clock's udevices are stored into UCLASS_CLK
>> list.
>>>>> With the ID we get proper udevice and from it and via driver_data
>>>>> the struct clk, which is then used in the CCF code to operate on
>>>>> clock devices (PLL, gate, mux, etc).
>>>>>
>>>>> I simply re-used the DM facilities (only missing is the back
>>>>> pointer from udevice to struct clk).
>>>>
>>>> Well then you can use dev_get_uclass_priv() for that.
>>>
>>> I think that it might be enough to use udevice's priv as clock
>>> specific structure (struct clk_gate2) has the struct clock embedded in it.
>>
>> Why do that? The uclass is specifically designed to hold data that is common
>> to the uclass.
>>
>> Regards,
>> Simon

Hi Peng,

> Hi All
> 
> Will we force to use CCF in future?

I do think yes (as fair as I can tell this is what the community
needs/agreed).

> And Is there possibility this feature would land in v2019.07? 

Do you mean this release? We are -rc3 now so it is IMHO too late.
(The v2019.10-rc1 seems reasonable).

The status is that Simon, had some comments for it and I need to
address them (however, recently I got less time to do that).

> If not, I'll try to convert i.MX8MM CLK to no-CCF DM CLK version,
> since i.MX8MM has been pending for long time.

The CCF was under discussion for some time for a reason - I would like
to avoid adding ad-hoc code and make it DM compliant (thanks Simon for
your feedback) and re-usable as much as possible.

(I will try to do some work on it during this week).

> 
> Thanks,
> Peng.
> > Subject: Re: [PATCH v3 00/11] clk: Port Linux common clock
> > framework [CCF] to U-boot (tag: 5.0-rc3)
> > 
> > Hi Lukasz,
> > 
> > On Tue, 21 May 2019 at 08:48, Lukasz Majewski <lukma@denx.de>
> > wrote:  
> > >
> > > Hi Simon,
> > >  
> > > > Hi Lukasz,
> > > >
> > > > On Sun, 19 May 2019 at 15:03, Lukasz Majewski <lukma@denx.de>  
> > wrote:  
> > > > >
> > > > > Hi Simon,
> > > > >  
> > > > > > Hi Lukasz,
> > > > > >
> > > > > > On Sat, 18 May 2019 at 15:28, Lukasz Majewski
> > > > > > <lukma@denx.de> wrote:  
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > This is not the newest patch set version of CCF (v3 vs.
> > > > > > > v4), but the comments/issues apply.
> > > > > > >  
> > > > > > > > Hi Lukasz,
> > > > > > > >
> > > > > > > > On Thu, 25 Apr 2019 at 04:30, Lukasz Majewski
> > > > > > > > <lukma@denx.de>
> > > > > > > > wrote:  
> > > > > > > > >
> > > > > > > > > This patch series brings the files from Linux kernel
> > > > > > > > > to provide clocks support as it is used on the Linux
> > > > > > > > > kernel with common clock framework [CCF] setup.
> > > > > > > > >
> > > > > > > > > This series also fixes several problems with current
> > > > > > > > > clocks and provides sandbox tests for functions
> > > > > > > > > addded to clk-uclass.c file.
> > > > > > > > >
> > > > > > > > > Design decisions/issues:
> > > > > > > > > =========================
> > > > > > > > >
> > > > > > > > > - U-boot's 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() now caches the previously read
> > > > > > > > > data (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). The 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.
> > > > > > > > >
> > > > > > > > > - The "central" structure of this patch series is
> > > > > > > > > struct udevice and its driver_data field contains the
> > > > > > > > > struct clk pointer (to the originally created one).
> > > > > > > > >
> > > > > > > > > - Up till now U-boot's driver model's CLK operates on
> > > > > > > > > udevice (main access to clock is by udevice ops)
> > > > > > > > >   In the CCF the access to struct clk (comprising
> > > > > > > > > pointer to
> > > > > > > > > *dev) is possible via dev_get_driver_data()
> > > > > > > > >
> > > > > > > > >   Storing back pointer (from udevice to struct clk) as
> > > > > > > > > driver_data is a convention for CCF.  
> > > > > > > >
> > > > > > > > Ick. Why not use uclass-private data to store this,
> > > > > > > > since every UCLASS_CLK device can have a parent.  
> > > > > > >
> > > > > > > The "private_data" field would be also a good place to
> > > > > > > store the back pointer from udevice to struct clk [*].
> > > > > > > The problem with CCF and udevice's priv pointer is
> > > > > > > explained just below: 
> > > > > > > >  
> > > > > > > > >
> > > > > > > > > - I could use *private_alloc_size to allocate driver's
> > > > > > > > > 'private" structures (dev->priv) for e.g. divider
> > > > > > > > > (struct clk_divider *divider) for IMX6Q clock, but
> > > > > > > > > this would change the original structure of the CCF
> > > > > > > > > code.  
> > > > > > >
> > > > > > > The original Linux's CCF code for iMX relies on using
> > > > > > > kmalloc internally:
> > > > > > >
> > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2
> > > > > > > F%2Felixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fc
> > > > > > >  
> > lk%2Fimx%2Fclk-gate2.c%23L139&amp;data=02%7C01%7Cpeng.fan%40nx  
> > > > > > >  
> > p.com%7C995c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92  
> > > > > > >  
> > cd99c5c301635%7C0%7C0%7C636940832249605030&amp;sdata=NJdvX7JfV  
> > > > > > >  
> > g2Qd8H%2FEmnzeNS1v5xhz4AaMRhSUpo7MxI%3D&amp;reserved=0  
> > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2
> > > > > > > F%2Felixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fc
> > > > > > >  
> > lk%2Fclk-divider.c%23L471&amp;data=02%7C01%7Cpeng.fan%40nxp.co  
> > > > > > >  
> > m%7C995c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99  
> > > > > > >  
> > c5c301635%7C0%7C0%7C636940832249605030&amp;sdata=iF66y5IKaQY69  
> > > > > > > vyFV%2BY5HSsZiKDb4s%2BN2yMPOBNDOqA%3D&amp;reserved=0
> > > > > > >
> > > > > > > By using driver_data I've avoided the need to make more
> > > > > > > changes to the original Linux code.
> > > > > > >
> > > > > > > I could use udevice's priv with automatic data allocation
> > > > > > > but then the CCF ported code would require more changes
> > > > > > > and considering the (from the outset) need to "fit" this
> > > > > > > code into U-Boot's DM, it drives away from the original
> > > > > > > Linux code.  
> > > > > >
> > > > > > Is the main change the need to cast driver_data?  
> > > > >
> > > > > The main change would be to remove the per clock device memory
> > > > > allocation code (with exit paths) from the original CCF code.
> > > > >
> > > > > This shall not be so difficult.
> > > > >  
> > > > > > Perhaps that could be
> > > > > > hidden in a helper function/macro, so that in U-Boot it can
> > > > > > hide the use of (struct clk_uc_priv
> > > > > > *)dev_get_uclass_priv(clk->dev))>parent  ?  
> > > > >
> > > > > Helper function would help to some extend as we operate on
> > > > > structures similar to:
> > > > >
> > > > > struct clk_gate2 {
> > > > >         struct clk clk;
> > > > >         void __iomem    *reg;
> > > > >         u8              bit_idx;
> > > > >         u8              cgr_val;
> > > > >         u8              flags;
> > > > > };
> > > > >
> > > > > The helper would return struct clk's address which is the
> > > > > same as struct's clk_gate2 (this is assured by C standard).
> > > > >  
> > > > > >  
> > > > > > >
> > > > > > >  
> > > > > > > > >
> > > > > > > > > The question is if it would be better to use
> > > > > > > > > private_alloc_size (and dev->private) or stay with
> > > > > > > > > driver_data. The former requires some rewritting in
> > > > > > > > > CCF original code (to remove (c)malloc, etc), but
> > > > > > > > > comply with u-boot DM. The latter allows re-using the
> > > > > > > > > CCF code as is, but introduces some convention
> > > > > > > > > special for CCF (I'm not sure thought if dev->priv is
> > > > > > > > > NOT another convention as well).  
> > > > > > > >
> > > > > > > > Yes I would like to avoid malloc() calls in drivers and
> > > > > > > > use the in-built mechanism.  
> > > > > > >
> > > > > > > I see your point.
> > > > > > >
> > > > > > > If the community agrees - I can rewrite the code to use
> > > > > > > such approach (but issues pointed out in [*] still apply).
> > > > > > >  
> > > > > > > >  
> > > > > > > > >
> > > > > > > > > - I've added the clk_get_parent(), which reads
> > > > > > > > > parent's dev->driver_data 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.
> > > > > > > > >
> > > > > > > > > - For tests I had to "emulate" CCF code structure to
> > > > > > > > > test functionality of clk_get_parent_rate() and
> > > > > > > > > clk_get_by_id(). Those functions will not work
> > > > > > > > > properly with "standard" (i.e. non CCF) clock
> > > > > > > > > setup(with not set dev->driver_data to struct clk).  
> > > > > > > >
> > > > > > > > Well I think we need a better approach for that anywat.
> > > > > > > > driver_data is used for getting something from the DT.  
> > > > > > >
> > > > > > > Maybe the name (driver_data) was a bit misleading then.
> > > > > > > For CCF it stores the back pointer to struct clk (as in
> > > > > > > fact it is a CCF's "driver data").  
> > > > > >
> > > > > > Well it seems like a hack to me. Perhaps there is a good
> > > > > > reason for it in Linux?  
> > > > >
> > > > > In Linux there is another approach - namely the struct clk
> > > > > (which is the main struct for clock management) has pointer
> > > > > to struct clk_core, which has pointer to parent(s).
> > > > >
> > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > > elixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fclk%2Fclk
> > > > > .c%23L43&amp;data=02%7C01%7Cpeng.fan%40nxp.com%7C995c4869  
> > e12e471bd  
> > > > >  
> > b5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C
> > 63694  
> > > > >  
> > 0832249605030&amp;sdata=63VsTs6JusNw%2FT4mBpfsFLUZKXtyTpZlYx99jy
> > 36  
> > > > > 6Mk%3D&amp;reserved=0
> > > > >
> > > > > In the case of U-Boot - the CCF wants to work on struct clk,
> > > > > but the _main_ data structure for U-Boot is struct udevice.
> > > > > Hence the need to have a back pointer (or force struct clk to
> > > > > have NOT pointer to udevice, but the udevice itself - then
> > > > > container_of would then do the trick).  
> > > >
> > > > The thing I don't understand is that I assumed there is no 1:1
> > > > correspondence from struct clk to struct udevice.  
> > >
> > > For CCF there is 1:1 match - i.e. each struct udevice has a
> > > matching struct clk.
> > >  
> > > > I thought that we
> > > > could have one clock device which supports lots of clk IDs (e.g.
> > > > 0-23).  
> > >
> > > On IMX CCF the clock ID is a unique number to refer to a clock.
> > >
> > > For example 163 is spi0, 164 is spi1, 165 is spi2, etc. The CCF
> > > uses this ID to get proper struct clk.
> > >
> > > In case of CCF port in U-Boot we use clk ID to get proper udevice
> > > by searching UCLASS_CLK devices linked together. Then the struct
> > > clk is read with:
> > > struct clk *clk = (struct clk *)dev_get_driver_data(dev); And we
> > > do have match when clk->id == id.
> > >
> > >
> > > Having one device supporting many IDs would be not compatible
> > > with CCF where each clock has its own instance of device and
> > > clock specific structure.  
> > 
> > OK good.
> >   
> > >  
> > > >  
> > > > >  
> > > > > > Or is it just convenience?  
> > > > >
> > > > > As stated above - Linux all necessary information has
> > > > > accessible from struct clk.  
> > > >
> > > > Sure, but we can always find the udevice from the clk.  
> > >
> > > Yes, correct.
> > >
> > > However clocks (represented as struct udevices) are linked in a
> > > single, common uclass (UCLASS_CLK).  
> > 
> > OK good.
> >   
> > >  
> > > >
> > > > If we require that clk == udevice then we can go back the other
> > > > way too, by using uclass-private data attached to each device.  
> > >
> > > That may work.
> > >
> > > The struct udevice's priv would hold clock specific data structure
> > > (like struct clk_gate2  - automatically allocated and released).
> > >
> > > And the uclass_priv would contain the pointer to struct clk itself
> > > (however, in CCF it is always the same as clock specific data
> > > structure
> > > - being the first field in the structure).  
> > 
> > Yes, or struct clk_uc_priv could contain a *member* which is a
> > pointer to struct clk.
> >   
> > >  
> > > >  
> > > > >  
> > > > > >  
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > NOTE:
> > > > > > >
> > > > > > > [*] - I do have a hard time to understand how struct clk
> > > > > > > shall work with struct udevice?
> > > > > > >
> > > > > > > In Linux or Barebox the struct clk is the "main"
> > > > > > > structure to hold the clock management data (like freq,
> > > > > > > ops, flags, parent/sibling relation, etc).  
> > > > > >
> > > > > > Yes U-Boot has a uniform struct udevice for every device and
> > > > > > struct uclass for every class.
> > > > > >
> > > > > > But the interesting thing here is that clocks have their own
> > > > > > parent/sibling relationships, quite independent from the
> > > > > > device tree.  
> > > > >
> > > > > But there would be no harm if we could re-use udevice for it.
> > > > > In the current CCF (v4) patch set each clk IP block (like mux
> > > > > or gate) is modelled as udevice:
> > > > >
> > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > >  
> > pastebin.com%2FuVmwv5FT&amp;data=02%7C01%7Cpeng.fan%40nxp.com
> > %7C99  
> > > > >  
> > 5c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > 35%  
> > > > >  
> > 7C0%7C0%7C636940832249605030&amp;sdata=XXjDjW9UBtSyuVpNZPeVzY
> > AVCrz  
> > > > > fcnMokz46tqQ1QEo%3D&amp;reserved=0  
> > > >
> > > > I don't see how you can do this...doesn't it mean changing the
> > > > parents of existing devices? E.g. if a SPI clock can come from
> > > > one of 4 parents, do you need to changes its parent in the
> > > > driver-model tree?  
> > >
> > > Yes. The hierarchy is build when the "generic" clock driver is
> > > probed (@
> > > clk/imx/clk-imx6q.c) by using the udevice's parent pointer.
> > >
> > > If afterwards I need to change clock parent, then I simply change
> > > parent pointers in udevices.  
> > 
> > Well if you do that, you need to update the sibling lists
> > (device->sibling_node). 
> > >  
> > > >  
> > > > >  
> > > > > >  
> > > > > > >
> > > > > > > A side observation - we now have three different
> > > > > > > implementations of struct clk in U-Boot :-) (two of which
> > > > > > > have *ops inside :-) )  
> > > > > >
> > > > > > Oh dear.
> > > > > >
> > > > > > The broadcom iMX ones needs to be converted.
> > > > > >  
> > > > > > >
> > > > > > > In the case of U-Boot's DM (./include/clk.h) it only has a
> > > > > > > _pointer_ to udevice (which means that I cannot get the
> > > > > > > struct clk easily from udevice with container_of()). The
> > > > > > > struct udevice has instead the *ops and *parent pointer
> > > > > > > (to another udevice).  
> > > > > >
> > > > > > Yes that's correct. The struct clk is actually a handle to
> > > > > > the clock, and includes an ID number.  
> > > > >
> > > > > You mean the ID number of the clock ?  
> > > >
> > > > Yes:
> > > >
> > > > struct clk {
> > > > struct udevice *dev;
> > > > /*
> > > > * Written by of_xlate. We assume a single id is enough for now.
> > > > In the
> > > > * future, we might add more fields here.
> > > > */
> > > > unsigned long id;
> > > > };
> > > >
> > > >  
> > > > >  
> > > > > >  
> > > > > > >
> > > > > > > Two problems:
> > > > > > >
> > > > > > > - Linux CCF code uses massively "struct clk" to handle
> > > > > > > clock operations (but not udevice)  
> > > > > >
> > > > > > OK.
> > > > > >  
> > > > > > >
> > > > > > > - There is no clear idea of how to implement struct clk
> > > > > > > *clk_get_parent(struct clk *) in U-Boot.  
> > > > > >
> > > > > > As above, it seems that this might need to be implemented. I
> > > > > > don't think the DM parent/child relationships are good
> > > > > > enough for clk, since they are not aware of the ID.
> > > > > >  
> > > > > > >
> > > > > > > The reason is that we lack clear information about which
> > > > > > > udevice's data field shall be used to store the back
> > > > > > > pointer from udevice to struct clk.
> > > > > > >
> > > > > > > Any hints and ideas are more than welcome.  
> > > > > >
> > > > > > I think it would be good to get Stephen Warren's thoughts on
> > > > > > this as he made the change to introduce struct clk.  
> > > > >
> > > > > Ok.
> > > > >  
> > > > > >
> > > > > > But at present clk_set_parent() is implemented by calling
> > > > > > into the driver. The uclass itself does not maintain
> > > > > > information about what is a parent of what.  
> > > > >
> > > > > Linux struct clk has easy access to its parent's struct clk.
> > > > > This is what is missing in U-Boot's DM.
> > > > >  
> > > > > >
> > > > > > Do we *need* to maintain this information in the uclass?  
> > > > >
> > > > > I think that we don't need. It would be enough to modify
> > > > > struct clk to has struct udevice embedded in it (as Linux has
> > > > > struct clk_core), not the pointer. Then we can use
> > > > > container_of to get clock and re-use struct udevice's parent
> > > > > pointer (and maybe UCLASS_CLK list of devices).  
> > > > > >
> > > > > > I think it would be prohibitively expensive to separate out
> > > > > > each individual clock into a separate device (udevice), but
> > > > > > that would work.  
> > > > >
> > > > > This is the approach as I use now in CCF v4:
> > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > >  
> > pastebin.com%2FuVmwv5FT&amp;data=02%7C01%7Cpeng.fan%40nxp.com
> > %7C99  
> > > > >  
> > 5c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c3016
> > 35%  
> > > > >  
> > 7C0%7C0%7C636940832249605030&amp;sdata=XXjDjW9UBtSyuVpNZPeVzY
> > AVCrz  
> > > > > fcnMokz46tqQ1QEo%3D&amp;reserved=0
> > > > >
> > > > > It is expensive but logically correct as each mux, gate, pll
> > > > > is the other CLK IP block (device).  
> > > >
> > > > OK I see. What is the cost? Is it acceptable for a boot
> > > > loader?  
> > >
> > > To make it really small we would need to use OF_PLATDATA.
> > >
> > > For e.g. iMX6Q - I'm able to boot with this CCF port running in
> > > both SPL and U-Boot proper.
> > >
> > > But, yes the cost is to have the larger binary as we do have
> > > larger section with udevices linker list.
> > >
> > > Original Linux CCF code uses ~1KiB dynamic table to store
> > > pointers to struct clk addressed by ID number. In U-Boot instead
> > > - I add those devices to UCLASS_CLK list of devices (as 1KiB is a
> > > lot for SPL).  
> > 
> > OK.
> >   
> > >  
> > > >  
> > > > >  
> > > > > >
> > > > > > The only other option I see is to create a sibling list and
> > > > > > parent pointer inside struct clk.  
> > > > >
> > > > > This would be the approach similar to Linux kernel approach.
> > > > >
> > > > > However, I don't know what were original needs of struct clk
> > > > > (as it did not have it). Maybe Stephen can shed some light on
> > > > > it?  
> > > >
> > > > Hopefully.
> > > >  
> > > > >  
> > > > > >
> > > > > > I suspect this will affect power domain also, although we
> > > > > > don't have that yet.  
> > > > >
> > > > > iMX8 has some clocks which needs to be always recalculated as
> > > > > they depend on power domains which can be disabled.  
> > > >
> > > > OK
> > > >  
> > > > >  
> > > > > >
> > > > > > Do you think there is a case for building this into DM
> > > > > > itself, such that devices can have a list of IDs for each
> > > > > > device, each with independent parent/child relationships?  
> > > > >
> > > > > For iMX6Q the ID of the clock is used to get proper clock in
> > > > > drivers (or from DTS). All clock's udevices are stored into
> > > > > UCLASS_CLK  
> > list.  
> > > > > With the ID we get proper udevice and from it and via
> > > > > driver_data the struct clk, which is then used in the CCF
> > > > > code to operate on clock devices (PLL, gate, mux, etc).
> > > > >
> > > > > I simply re-used the DM facilities (only missing is the back
> > > > > pointer from udevice to struct clk).  
> > > >
> > > > Well then you can use dev_get_uclass_priv() for that.  
> > >
> > > I think that it might be enough to use udevice's priv as clock
> > > specific structure (struct clk_gate2) has the struct clock
> > > embedded in it.  
> > 
> > Why do that? The uclass is specifically designed to hold data that
> > is common to the uclass.
> > 
> > Regards,
> > Simon  




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@denx.de

Hi Lukasz,

On Mon, 3 Jun 2019 at 08:22, Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Peng,
>
> > Hi All
> >
> > Will we force to use CCF in future?
>
> I do think yes (as fair as I can tell this is what the community
> needs/agreed).
>
> > And Is there possibility this feature would land in v2019.07?
>
> Do you mean this release? We are -rc3 now so it is IMHO too late.
> (The v2019.10-rc1 seems reasonable).
>
> The status is that Simon, had some comments for it and I need to
> address them (however, recently I got less time to do that).
>
> > If not, I'll try to convert i.MX8MM CLK to no-CCF DM CLK version,
> > since i.MX8MM has been pending for long time.
>
> The CCF was under discussion for some time for a reason - I would like
> to avoid adding ad-hoc code and make it DM compliant (thanks Simon for
> your feedback) and re-usable as much as possible.
>
> (I will try to do some work on it during this week).

Just checking on the state of this since I have been a bit out of
touch for a while.

Regards,
Simon