Quoting Alex Elder (2014-05-29 09:53:50)

On 05/29/2014 11:35 AM, Mike Turquette wrote:

quoted

Quoting Alex Elder (2014-05-29 06:26:15)

quoted

On 05/23/2014 07:53 PM, Mike Turquette wrote:

quoted

The above seems like a lot effort to go to. Why not skip all of this and
just implement the prerequisite logic in the .enable & .disable
callbacks? E.g. your kona clk .enable callback would look like:

I think the problem is that it means the clock consumers
would have to know that prerequisite relationship.  And
that is dependent on the clock tree.  The need for it in
this case was because the boot loader didn't initialize
all the clocks that were needed.  If we could count on
the boot loader setting things up initially we might not
need to do this.

I think you've convinced me that if the prerequisite is
set up at initialization time, the consumers don't need
to know about the the clock tree.

quoted

quoted

quoted

diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c
index d603c4e..51f35b4 100644
--- a/drivers/clk/bcm/clk-kona.c
+++ b/drivers/clk/bcm/clk-kona.c

@@ -987,6 +987,12 @@ static int kona_peri_clk_enable(struct clk_hw *hw)
 {
      struct kona_clk *bcm_clk = to_kona_clk(hw);
      struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
+     int ret;
+
+     hw->prereq_bus_clk = clk_get(hw->ccu, hw->prereq);
+     ret = clk_enable(prereq_bus_clk);
+     if (ret)
+             return ret;
 
      return clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, true);
 }

@@ -997,6 +1003,9 @@ static void kona_peri_clk_disable(struct clk_hw *hw)
      struct bcm_clk_gate *gate = &bcm_clk->u.peri->gate;
 
      (void)clk_gate(bcm_clk->ccu, bcm_clk->init_data.name, gate, false);
+
+     clk_disable(hw->prereq_bus_clk);
+     clk_put(hw->prereq_bus_clk);
 }
 
 static int kona_peri_clk_is_enabled(struct clk_hw *hw)

I guess it might take some trickery to get clk_get to work like that.
Let me know if I've completely lost the plot.

I don't think so, but I think there's a lot of stuff
here to try to understand, and you're trying to extract
it from the code without the benefit of some background
of how and why it's done this way.

Hopefully all this verbiage is moving you closer to
understanding...  I appreciate your patience.

Hi Alex,

Can you comment on my diff above? I basically tossed up some pseudo-code
to show how clk_enable calls can be nested inside of each other. I'd
like to know if that approach makes sense for your prereq clocks case.

Yes, I should have looked more closely before.

Are you suggesting this prerequisite notion get elevated into the
common framework?

Nope.

Or is "hw" here just representative of the
Kona-specific clock structure?

Yup. It's just good old struct clk_hw. There is one instance of this
struct for every struct clk object.

In any case, you're suggesting the prerequisite be handled in the
enable path (as opposed to the one-time initialization path),
which during the course of this discussion I've been thinking may
be the right way to do it.

Right, and don't forget that you have both the prepare path AND the
enable path. It is common for drivers to call clk_prepare once at probe
time and then aggressively call clk_enable/clk_disable for fine-grained
PM. Likewise some drivers always use clk_prepare_enable and
clk_disable_unprepare.

The point is that you have two callbacks that you might split some of
this stuff across. Your "initializiation" stuff might go into .prepare()
and simply enabling the clock might go into .enable().

Let me see if I can rework it that way and I'll let you know
what I discover as a result.  I hope to have something to
talk about later today.

Sounds good.

Regards,
Mike

Thanks a lot Mike.

                                        -Alex

quoted

Note that Linux device drivers that consume leaf clocks do NOT need to
know about the prereq clocks. All of that prereq clock knowledge is
stored in the .enable callback for the leaf clock (see above).

Regards,
Mike

quoted

                                        -Alex