[U-Boot] [PATCH v3 00/11] clk: Port Linux common clock framework [CCF] to U-boot (tag: 5.0-rc3)
Lukasz Majewski
lukma at denx.de
Mon Jun 3 07:22:47 UTC 2019
Hi Peng,
> Hi All
>
> Will we force to use CCF in future?
I do think yes (as fair as I can tell this is what the community
needs/agreed).
> And Is there possibility this feature would land in v2019.07?
Do you mean this release? We are -rc3 now so it is IMHO too late.
(The v2019.10-rc1 seems reasonable).
The status is that Simon, had some comments for it and I need to
address them (however, recently I got less time to do that).
> 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.
The CCF was under discussion for some time for a reason - I would like
to avoid adding ad-hoc code and make it DM compliant (thanks Simon for
your feedback) and re-usable as much as possible.
(I will try to do some work on it during this week).
>
> 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,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190603/f2c61a9a/attachment.sig>
More information about the U-Boot
mailing list