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