Hi Lukasz, On Sun, 19 May 2019 at 15:03, Lukasz Majewski <lu...@denx.de> wrote: > > Hi Simon, > > > 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? > > 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).
The thing I don't understand is that I assumed there is no 1:1 correspondence from struct clk to struct udevice. I thought that we could have one clock device which supports lots of clk IDs (e.g. 0-23). > > > Or is it just convenience? > > As stated above - Linux all necessary information has accessible from > struct clk. Sure, but we can always find the udevice from the clk. If we require that clk == udevice then we can go back the other way too, by using uclass-private data attached to each device. > > > > > > > > > > > > > > > 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 I don't see how you can do this...doesn't it mean changing the parents of existing devices? E.g. if a SPI clock can come from one of 4 parents, do you need to changes its parent in the driver-model 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. > > You mean the ID number of the clock ? Yes: struct clk { struct udevice *dev; /* * Written by of_xlate. We assume a single id is enough for now. In the * future, we might add more fields here. */ unsigned long id; }; > > > > > > > > > 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). OK I see. What is the cost? Is it acceptable for a boot loader? > > > > > 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? Hopefully. > > > > > 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. OK > > > > > 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). Well then you can use dev_get_uclass_priv() for that. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
