Re: [PATCH v4 0/4] Add device links to clocks

[Miquel Raynal]

Hi Stephen,

Stephen Boyd <sboyd@xxxxxxxxxx> wrote on Thu, 11 Apr 2019 16:34:16
-0700:

> Quoting Miquel Raynal (2019-01-08 08:19:36)
> > Hello,
> > 
> > While working on suspend to RAM feature, I ran into troubles multiple
> > times when clocks where not suspending/resuming at the desired time. I
> > had a look at the core and I think the same logic as in the
> > regulator's core may be applied here to (very easily) fix this issue:
> > using device links.
> > 
> > The only additional change I had to do was to always (when available)
> > populate the device entry of the core clock structure so that it could
> > be used later. This is the purpose of patch 1. Patch 2 actually adds
> > support for device links.
> > 
> > Here is a step-by-step explanation of how links are managed, following
> > Maxime Ripard's suggestion.
> > 
> > 
> > The order of probe has no importance because the framework already
> > handles orphaned clocks so let's be simple and say there are two root
> > clocks, not depending on anything, that are probed first: xtal0 and
> > xtal1. None of these clocks have a parent, there is no device link in
> > the game, yet.
> > 
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |                |            |                |
> >    |   xtal0 core   |            |   xtal1 core   |
> >    |                |            |                |
> >    |                |            |                |
> >    +-------^^-------+            +-------^^-------+
> >            ||                            ||
> >            ||                            ||
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |   xtal0 clk    |            |   xtal1 clk    |
> >    |                |            |                |
> >    +----------------+            +----------------+
> > 
> > Then, a peripheral clock periph0 is probed. His parent is xtal1. The
> > clock_register_*() call will run __clk_init_parent() and a link between
> > periph0's core and xtal1's core will be created and stored in
> > periph0's core->parent_clk_link entry.
> > 
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |                |            |                |
> >    |   xtal0 core   |            |   xtal1 core   |
> >    |                |            |                |
> >    |                |            |                |
> >    +-------^^-------+            +-------^^-------+
> >            ||                            ||
> >            ||                            ||
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |   xtal0 clk    |            |   xtal1 clk    |
> >    |                |            |                |
> >    +----------------+            +-------^--------+
> >                                          |
> >                                          |
> >                           +--------------+
> >                           |   ->parent_clk_link
> >                           |
> >                   +----------------+
> >                   |                |
> >                   |                |
> >                   |  periph0 core  |
> >                   |                |
> >                   |                |
> >                   +-------^^-------+
> >                           ||
> >                           ||
> >                   +----------------+
> >                   |                |
> >                   |  periph0 clk 0 |
> >                   |                |
> >                   +----------------+
> > 
> > Then, device0 is probed and "get" the periph0 clock. clk_get() will be
> > called and a struct clk will be instantiated for device0 (called in
> > the figure clk 1). A link between device0 and the new clk 1 instance of
> > periph0 will be created and stored in the clk->consumer_link entry.
> > 
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |                |            |                |
> >    |   xtal0 core   |            |   xtal1 core   |
> >    |                |            |                |
> >    |                |            |                |
> >    +-------^^-------+            +-------^^-------+
> >            ||                            ||
> >            ||                            ||
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |   xtal0 clk    |            |   xtal1 clk    |
> >    |                |            |                |
> >    +----------------+            +-------^--------+
> >                                          |
> >                                          |
> >                           +--------------+
> >                           |   ->parent_clk_link
> >                           |
> >                   +----------------+
> >                   |                |
> >                   |                |
> >                   |  periph0 core  |
> >                   |                <-------------+
> >                   |                <-------------|
> >                   +-------^^-------+            ||
> >                           ||                    ||
> >                           ||                    ||
> >                   +----------------+    +----------------+
> >                   |                |    |                |
> >                   |  periph0 clk 0 |    |  periph0 clk 1 |
> >                   |                |    |                |
> >                   +----------------+    +----------------+
> >                                                 |
> >                                                 | ->consumer_link
> >                                                 |
> >                                                 |
> >                                                 |
> >                                         +-------v--------+
> >                                         |    device0     |
> >                                         +----------------+
> > 
> > Right now, device0 is linked to periph0, itself linked to xtal1 so
> > everything is fine.
> > 
> > Now let's get some fun: the new parent of periph0 is xtal1. The process
> > will call clk_reparent(), periph0's core->parent_clk_link will be
> > destroyed and a new link to xtal1 will be setup and stored. The
> > situation is now that device0 is linked to periph0 and periph0 is
> > linked to xtal1, so the dependency between device0 and xtal1 is still
> > clear.
> > 
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |                |            |                |
> >    |   xtal0 core   |            |   xtal1 core   |
> >    |                |            |                |
> >    |                |            |                |
> >    +-------^^-------+            +-------^^-------+
> >            ||                            ||
> >            ||                            ||
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |   xtal0 clk    |            |   xtal1 clk    |
> >    |                |            |                |
> >    +-------^--------+            +----------------+
> >            |
> >            |                           \ /
> >            +----------------------------x  
> >       ->parent_clk_link   |            / \  
> >                           |
> >                   +----------------+
> >                   |                |
> >                   |                |
> >                   |  periph0 core  |
> >                   |                <-------------+
> >                   |                <-------------|
> >                   +-------^^-------+            ||
> >                           ||                    ||
> >                           ||                    ||
> >                   +----------------+    +----------------+
> >                   |                |    |                |
> >                   |  periph0 clk 0 |    |  periph0 clk 1 |
> >                   |                |    |                |
> >                   +----------------+    +----------------+
> >                                                 |
> >                                                 | ->consumer_link
> >                                                 |
> >                                                 |
> >                                                 |
> >                                         +-------v--------+
> >                                         |    device0     |
> >                                         +----------------+
> > 
> > I assume periph0 cannot be removed while there are devices using it,
> > same for xtal0.
> > 
> > What can happen is that device0 'put' the clock periph0. The relevant
> > link is deleted and the clk instance dropped.
> > 
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |                |            |                |
> >    |   xtal0 core   |            |   xtal1 core   |
> >    |                |            |                |
> >    |                |            |                |
> >    +-------^^-------+            +-------^^-------+
> >            ||                            ||
> >            ||                            ||
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |   xtal0 clk    |            |   xtal1 clk    |
> >    |                |            |                |
> >    +-------^--------+            +----------------+
> >            |
> >            |                           \ /
> >            +----------------------------x  
> >       ->parent_clk_link   |            / \  
> >                           |
> >                   +----------------+
> >                   |                |
> >                   |                |
> >                   |  periph0 core  |
> >                   |                |
> >                   |                |
> >                   +-------^^-------+
> >                           ||
> >                           ||
> >                   +----------------+
> >                   |                |
> >                   |  periph0 clk 0 |
> >                   |                |
> >                   +----------------+
> > 
> > Now we can unregister periph0: link with the parent will be destroyed
> > and the clock may be safely removed.
> > 
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |                |            |                |
> >    |   xtal0 core   |            |   xtal1 core   |
> >    |                |            |                |
> >    |                |            |                |
> >    +-------^^-------+            +-------^^-------+
> >            ||                            ||
> >            ||                            ||
> >    +----------------+            +----------------+
> >    |                |            |                |
> >    |   xtal0 clk    |            |   xtal1 clk    |
> >    |                |            |                |
> >    +----------------+            +----------------+
> > 
> > 
> > This is my understanding of the common clock framework and how links
> > can be added to it.
> > 
> > As a result, here are the links created during the boot of an
> > ESPRESSObin:
> >   
> 
> Sorry this patch series is taking way too long to get merged. It's
> already mid-April!
> 
> So I still have some of the original questions I had from before, mostly
> around circular parent chains between clk providers. For example, there
> are clk providers that both provide clks to other providers and consume
> clks from those providers. Does device links work gracefully here?
> 
> Just speaking from my own qcom experience, I can point to the PCIe PHY
> that's a provider of a clk to GCC and a consumer of a clk in GCC. In
> block diagram form this is:
> 
> 
>       PCIE PHY                        GCC
>    +--------------+          +-------------------------+
>    |              |          |                         |
>    |     PHY clk ->----------+---- gcc_pipe_clk ---+   |
>    |              |          |                     |   |
>    |              |          |                     |   |
>    | pci_pipe_clk <----------|---------------------+   |
>    |              |          |                         |
>    +--------------+          +-------------------------+
> 
> The end result is that the PCIe PHY is a clk controller that provides
> the PHY clk to GCC's gcc_pipe_clk and then it gets the same clk signal
> back from GCC and uses it on the PCIe PHY's pci_pipe_clk input.
> 
> So is this is a problem?
> 

It's now my turn to get back on this topic.

I just put my noise back into this and for what I understand of the
clk subsystem, I think the situation you describe could be pictured
like this:


         +---------------+
         |               |
         |               |
         | PCIe PHY      |
         |               |
         |               |
         +-----^^--------+
               ||
               ||
         +---------------+
         |               |
         | pcie_pipe_clk |
         |               |
         +------^--------+
                |
                | ->parent_clk_link
                |
                |
         +---------------+
         |               |
         |               |
         | GCC           |
         |               |
         |               |
         +------^^-------+
                ||
                ||
         +---------------+
         |               |
         | gcc_pipe_clk  |
         |               |
         +------^--------+
                |
                | ->parent_clk_link
                |
                |
         +---------------+
         |               |
         |               |
         | PCIe PHY      |
         |               |
         |               |
         +------^^-------+
                ||
                ||
         +---------------+
         |               |
         | phy_clk       |
         |               |
         +---------------+


IMHO the fact that the first and third blocks are the same does not
interfere with device links.

Honestly, I cannot be 100% sure it won't break on qcom designs, maybe
the best would be to have someone to test. I don't have the relevant
hardware. Do you? It would be really helpful!

There is an entire PCIe series blocked, waiting for these device links
to be merged so it would help a lot if someone could test.

Thank you very much,
MiquÃl