Hi All Will we force to use CCF in future? And Is there possibility this feature would land in v2019.07? If not, I'll try to convert i.MX8MM CLK to no-CCF DM CLK version, since i.MX8MM has been pending for long time.
Thanks, Peng. > Subject: Re: [PATCH v3 00/11] clk: Port Linux common clock framework [CCF] > to U-boot (tag: 5.0-rc3) > > Hi Lukasz, > > On Tue, 21 May 2019 at 08:48, Lukasz Majewski <lu...@denx.de> wrote: > > > > Hi Simon, > > > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2 > > > > > > F%2Felixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fc > > > > > > > lk%2Fimx%2Fclk-gate2.c%23L139&data=02%7C01%7Cpeng.fan%40nx > > > > > > > p.com%7C995c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92 > > > > > > > cd99c5c301635%7C0%7C0%7C636940832249605030&sdata=NJdvX7JfV > > > > > > > g2Qd8H%2FEmnzeNS1v5xhz4AaMRhSUpo7MxI%3D&reserved=0 > > > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2 > > > > > > F%2Felixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fc > > > > > > > lk%2Fclk-divider.c%23L471&data=02%7C01%7Cpeng.fan%40nxp.co > > > > > > > m%7C995c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99 > > > > > > > c5c301635%7C0%7C0%7C636940832249605030&sdata=iF66y5IKaQY69 > > > > > > vyFV%2BY5HSsZiKDb4s%2BN2yMPOBNDOqA%3D&reserved=0 > > > > > > > > > > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > elixir.bootlin.com%2Flinux%2Fv5.1.2%2Fsource%2Fdrivers%2Fclk%2Fclk > > > > .c%23L43&data=02%7C01%7Cpeng.fan%40nxp.com%7C995c4869 > e12e471bd > > > > > b5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C > 63694 > > > > > 0832249605030&sdata=63VsTs6JusNw%2FT4mBpfsFLUZKXtyTpZlYx99jy > 36 > > > > 6Mk%3D&reserved=0 > > > > > > > > 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. > > > > For CCF there is 1:1 match - i.e. each struct udevice has a matching > > struct clk. > > > > > I thought that we > > > could have one clock device which supports lots of clk IDs (e.g. > > > 0-23). > > > > On IMX CCF the clock ID is a unique number to refer to a clock. > > > > For example 163 is spi0, 164 is spi1, 165 is spi2, etc. The CCF uses > > this ID to get proper struct clk. > > > > In case of CCF port in U-Boot we use clk ID to get proper udevice by > > searching UCLASS_CLK devices linked together. Then the struct clk is > > read with: > > struct clk *clk = (struct clk *)dev_get_driver_data(dev); And we do > > have match when clk->id == id. > > > > > > Having one device supporting many IDs would be not compatible with CCF > > where each clock has its own instance of device and clock specific > > structure. > > OK good. > > > > > > > > > > > > > > > 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. > > > > Yes, correct. > > > > However clocks (represented as struct udevices) are linked in a > > single, common uclass (UCLASS_CLK). > > OK good. > > > > > > > > > If we require that clk == udevice then we can go back the other way > > > too, by using uclass-private data attached to each device. > > > > That may work. > > > > The struct udevice's priv would hold clock specific data structure > > (like struct clk_gate2 - automatically allocated and released). > > > > And the uclass_priv would contain the pointer to struct clk itself > > (however, in CCF it is always the same as clock specific data > > structure > > - being the first field in the structure). > > Yes, or struct clk_uc_priv could contain a *member* which is a pointer to > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > > pastebin.com%2FuVmwv5FT&data=02%7C01%7Cpeng.fan%40nxp.com > %7C99 > > > > > 5c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c3016 > 35% > > > > > 7C0%7C0%7C636940832249605030&sdata=XXjDjW9UBtSyuVpNZPeVzY > AVCrz > > > > fcnMokz46tqQ1QEo%3D&reserved=0 > > > > > > 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? > > > > Yes. The hierarchy is build when the "generic" clock driver is probed > > (@ > > clk/imx/clk-imx6q.c) by using the udevice's parent pointer. > > > > If afterwards I need to change clock parent, then I simply change > > parent pointers in udevices. > > Well if you do that, you need to update the sibling lists > (device->sibling_node). > > > > > > > > > > > > > > > > > > > > > > > > > > > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2F > > > > > pastebin.com%2FuVmwv5FT&data=02%7C01%7Cpeng.fan%40nxp.com > %7C99 > > > > > 5c4869e12e471bdb5108d6de4feda5%7C686ea1d3bc2b4c6fa92cd99c5c3016 > 35% > > > > > 7C0%7C0%7C636940832249605030&sdata=XXjDjW9UBtSyuVpNZPeVzY > AVCrz > > > > fcnMokz46tqQ1QEo%3D&reserved=0 > > > > > > > > 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? > > > > To make it really small we would need to use OF_PLATDATA. > > > > For e.g. iMX6Q - I'm able to boot with this CCF port running in both > > SPL and U-Boot proper. > > > > But, yes the cost is to have the larger binary as we do have larger > > section with udevices linker list. > > > > Original Linux CCF code uses ~1KiB dynamic table to store pointers to > > struct clk addressed by ID number. In U-Boot instead - I add those > > devices to UCLASS_CLK list of devices (as 1KiB is a lot for SPL). > > OK. > > > > > > > > > > > > > > > > > > > > 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. > > > > I think that it might be enough to use udevice's priv as clock > > specific structure (struct clk_gate2) has the struct clock embedded in it. > > Why do that? The uclass is specifically designed to hold data that is common > to the uclass. > > Regards, > Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
