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

[Lukasz Majewski <lukma@denx.de>]

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 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/20190519/3d52b8b7/attachment.sig>