On 5/30/19 5:04 AM, Peng Fan wrote: > Hi All Hi,
> Will we force to use CCF in future? Force how ? > And Is there possibility this feature would land in v2019.07? No, it's rc3 already, no way. > 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. Good Also please do not top-post > 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 -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot