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#L... https://elixir.bootlin.com/linux/v5.1.2/source/drivers/clk/clk-divider.c#L47...
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:
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