Hi Lukasz, On Sat, 18 May 2019 at 15:28, Lukasz Majewski <lu...@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 <lu...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot