[U-Boot] [PATCH v3 00/11] clk: Port Linux common clock framework [CCF] to U-boot (tag: 5.0-rc3)

[Simon Glass]

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? 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  ?

>
>
> > >
> > > 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? Or is it just convenience?

>
>
>
> 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.

>
> 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.

>
> 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.

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.

Do we *need* to maintain this information in the uclass?

I think it would be prohibitively expensive to separate out each
individual clock into a separate device (udevice), but that would
work.

The only other option I see is to create a sibling list and parent
pointer inside struct clk.

I suspect this will affect power domain also, although we don't have that yet.

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?

Regards,
Simon