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

[Lukasz Majewski]

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 at 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 at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190518/1540442b/attachment.sig>