[U-Boot] [PATCH v3 00/11] clk: Port Linux common clock framework [CCF] to U-boot (tag: 5.0-rc3)
Marek Vasut
marex at denx.de
Thu May 30 08:17:15 UTC 2019
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 <lukma at denx.de> wrote:
>>>
>>> Hi Simon,
>>>
>>>> Hi Lukasz,
>>>>
>>>> On Sun, 19 May 2019 at 15:03, Lukasz Majewski <lukma at denx.de>
>> wrote:
>>>>>
>>>>> Hi Simon,
>>>>>
>>>>>> Hi Lukasz,
>>>>>>
>>>>>> On Sat, 18 May 2019 at 15:28, Lukasz Majewski <lukma at 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
>>>>>>>> <lukma at 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
More information about the U-Boot
mailing list