[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
Tue May 21 14:48:15 UTC 2019
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://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.
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.
>
> >
> > > 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).
>
> 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).
>
> >
> > >
> > > >
> > > >
> > > >
> > > > 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?
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.
>
> >
> > >
> > > >
> > > > 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?
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).
>
> >
> > >
> > > 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.
>
> 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/20190521/8fa0a5cb/attachment.sig>
More information about the U-Boot
mailing list