| Message ID | 20140524005358.23136.8918@quantum (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/23/2014 07:53 PM, Mike Turquette wrote: > Quoting Alex Elder (2014-05-20 05:52:39) >> @@ -743,11 +746,16 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk) >> clk = clk_register(NULL, &bcm_clk->hw); >> if (IS_ERR(clk)) { >> pr_err("%s: error registering clock %s (%ld)\n", __func__, >> - init_data->name, PTR_ERR(clk)); >> + name, PTR_ERR(clk)); >> goto out_teardown; >> } >> BUG_ON(!clk); >> >> + /* Make it so we can look the clock up using clk_find() */ > > s/clk_find/clk_get/ ? Yes, this is a mistake. > >> + bcm_clk->cl.con_id = name; >> + bcm_clk->cl.clk = clk; >> + clkdev_add(&bcm_clk->cl); > > This is not so nice. I'll explain more below. Actually, this code is no longer needed at all. It was at one time, but I evolved away from that need, and never noticed that this remnant remained. I will delete it. I'm really sorry I missed that, it was confusing for it to still be there I'm sure. >> + >> return clk; >> out_teardown: >> bcm_clk_teardown(bcm_clk); >> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c >> index d8a7f38..fd070d6 100644 >> --- a/drivers/clk/bcm/clk-kona.c >> +++ b/drivers/clk/bcm/clk-kona.c >> @@ -1195,6 +1195,48 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk) >> return true; >> } >> >> +static bool __kona_clk_init(struct kona_clk *bcm_clk); >> +static bool __kona_prereq_init(struct kona_clk *bcm_clk) >> +{ >> + struct clk *clk; >> + struct clk_hw *hw; >> + struct kona_clk *prereq; >> + >> + BUG_ON(clk_is_initialized(bcm_clk)); >> + >> + if (!bcm_clk->p.prereq) >> + return true; >> + >> + clk = clk_get(NULL, bcm_clk->p.prereq); > > The clkdev global namespace is getting polluted with all of these new > prereq clocks. If there was an associated struct device *dev with them > then it wouldn't be a problem, but you might get collisions with other > clock drivers that also use NULL for the device. Again, you caught a confusing mistake. The clk_lookup structure will go away. > It would be a lot nicer for the clocks that require a prereq clock to > just use clk_get(dev, "well_known_name"); in the same way that drivers > use it, without considering it a special case. That is in fact what happens, in __kona_prereq_init(). >> + if (IS_ERR(clk)) { >> + pr_err("%s: unable to get prereq clock %s for %s\n", >> + __func__, bcm_clk->p.prereq, bcm_clk->init_data.name); >> + return false; >> + } >> + hw = __clk_get_hw(clk); >> + if (!hw) { >> + pr_err("%s: null hw pointer for clock %s\n", __func__, >> + bcm_clk->init_data.name); >> + return false; >> + } >> + prereq = to_kona_clk(hw); >> + if (prereq->ccu != bcm_clk->ccu) { >> + pr_err("%s: prereq clock %s CCU different for clock %s\n", >> + __func__, bcm_clk->p.prereq, bcm_clk->init_data.name); >> + return false; >> + } >> + >> + /* Initialize the prerequisite clock first */ >> + if (!__kona_clk_init(prereq)) { >> + pr_err("%s: failed to init prereq %s for clock %s\n", >> + __func__, bcm_clk->p.prereq, bcm_clk->init_data.name); >> + return false; >> + } >> + bcm_clk->p.prereq_clk = clk; > > 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. > > 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. -Alex
Quoting Alex Elder (2014-05-29 06:26:15) > On 05/23/2014 07:53 PM, Mike Turquette wrote: > > 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. > > > > > 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. 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 > > -Alex >
On 05/29/2014 11:35 AM, Mike Turquette wrote: > Quoting Alex Elder (2014-05-29 06:26:15) >> On 05/23/2014 07:53 PM, Mike Turquette wrote: >>> 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. >>> >>> 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? Or is "hw" here just representative of the Kona-specific clock structure? 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. 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. Thanks a lot Mike. -Alex > 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 > >> >> -Alex >>
Quoting Alex Elder (2014-05-29 09:53:50) > On 05/29/2014 11:35 AM, Mike Turquette wrote: > > Quoting Alex Elder (2014-05-29 06:26:15) > >> On 05/23/2014 07:53 PM, Mike Turquette wrote: > >>> 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. > > >>> > >>> 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 > > > 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 > > > >> > >> -Alex > >> >
On 05/23/2014 07:53 PM, Mike Turquette wrote: > Quoting Alex Elder (2014-05-20 05:52:39) >> @@ -743,11 +746,16 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk) >> clk = clk_register(NULL, &bcm_clk->hw); >> if (IS_ERR(clk)) { >> pr_err("%s: error registering clock %s (%ld)\n", __func__, >> - init_data->name, PTR_ERR(clk)); >> + name, PTR_ERR(clk)); >> goto out_teardown; >> } >> BUG_ON(!clk); >> >> + /* Make it so we can look the clock up using clk_find() */ > > s/clk_find/clk_get/ ? > >> + bcm_clk->cl.con_id = name; >> + bcm_clk->cl.clk = clk; >> + clkdev_add(&bcm_clk->cl); > > This is not so nice. I'll explain more below. OK, despite what I said before, I do need this, or something like it, so I can look up clocks by name. (Continued below.) > >> + >> return clk; >> out_teardown: >> bcm_clk_teardown(bcm_clk); >> diff --git a/drivers/clk/bcm/clk-kona.c b/drivers/clk/bcm/clk-kona.c >> index d8a7f38..fd070d6 100644 >> --- a/drivers/clk/bcm/clk-kona.c >> +++ b/drivers/clk/bcm/clk-kona.c >> @@ -1195,6 +1195,48 @@ static bool __peri_clk_init(struct kona_clk *bcm_clk) >> return true; >> } >> >> +static bool __kona_clk_init(struct kona_clk *bcm_clk); >> +static bool __kona_prereq_init(struct kona_clk *bcm_clk) >> +{ >> + struct clk *clk; >> + struct clk_hw *hw; >> + struct kona_clk *prereq; >> + >> + BUG_ON(clk_is_initialized(bcm_clk)); >> + >> + if (!bcm_clk->p.prereq) >> + return true; >> + >> + clk = clk_get(NULL, bcm_clk->p.prereq); > > The clkdev global namespace is getting polluted with all of these new > prereq clocks. If there was an associated struct device *dev with them > then it wouldn't be a problem, but you might get collisions with other > clock drivers that also use NULL for the device. Yes I recognize this. Ideally a CCU would have a device struct associated with it that I could use, because the name of a clock is unique within that context. But I have no such device available. (Please correct me if I'm wrong. I don't want to make one up, and I would like to use it if it exists.) > It would be a lot nicer for the clocks that require a prereq clock to > just use clk_get(dev, "well_known_name"); in the same way that drivers > use it, without considering it a special case. I can do something like that if I can get a meaningful device structure. Do you have any suggestions? Other than this issue, I've implemented all of the previous initialization routines using ->prepare() instead, and it works fine. I'm going to send an updated series out tomorrow. I want to look it over again after a good night's sleep... -Alex >> + if (IS_ERR(clk)) { >> + pr_err("%s: unable to get prereq clock %s for %s\n", >> + __func__, bcm_clk->p.prereq, bcm_clk->init_data.name); >> + return false; >> + } >> + hw = __clk_get_hw(clk); >> + if (!hw) { >> + pr_err("%s: null hw pointer for clock %s\n", __func__, >> + bcm_clk->init_data.name); >> + return false; >> + } >> + prereq = to_kona_clk(hw); >> + if (prereq->ccu != bcm_clk->ccu) { >> + pr_err("%s: prereq clock %s CCU different for clock %s\n", >> + __func__, bcm_clk->p.prereq, bcm_clk->init_data.name); >> + return false; >> + } >> + >> + /* Initialize the prerequisite clock first */ >> + if (!__kona_clk_init(prereq)) { >> + pr_err("%s: failed to init prereq %s for clock %s\n", >> + __func__, bcm_clk->p.prereq, bcm_clk->init_data.name); >> + return false; >> + } >> + bcm_clk->p.prereq_clk = clk; > > 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: > > 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. > > Regards, > Mike >
On 05/29/2014 10:20 PM, Alex Elder wrote: > On 05/23/2014 07:53 PM, Mike Turquette wrote: >> Quoting Alex Elder (2014-05-20 05:52:39) >>> @@ -743,11 +746,16 @@ struct clk *kona_clk_setup(struct kona_clk *bcm_clk) >>> clk = clk_register(NULL, &bcm_clk->hw); >>> if (IS_ERR(clk)) { >>> pr_err("%s: error registering clock %s (%ld)\n", __func__, >>> - init_data->name, PTR_ERR(clk)); >>> + name, PTR_ERR(clk)); >>> goto out_teardown; >>> } >>> BUG_ON(!clk); >>> >>> + /* Make it so we can look the clock up using clk_find() */ >> >> s/clk_find/clk_get/ ? >> >>> + bcm_clk->cl.con_id = name; >>> + bcm_clk->cl.clk = clk; >>> + clkdev_add(&bcm_clk->cl); >> >> This is not so nice. I'll explain more below. > > OK, despite what I said before, I do need this, or > something like it, so I can look up clocks by name. > (Continued below.) ... I've been thinking this morning about ways to at least improve this. The problem is worse than just prerequisite clocks polluting the global name space. Right now *all* clocks get their name registered this way, because any one of them could be tagged as a prerequisite, and therefore in need of lookup by name. If I had a device structure to associate the clock names with it would help, but I don't have one. There is no other way to define a separate name space, it's either associated with a device, or it's global. Given all that, I could prefix or suffix the clock names with some special string, in order to sort of carve out a reserved portion of the global name space. I could specify the prerequisite clock by its index in its CCU's clocks array. I could then manufacture a of_phandle_args structure and use of_clk_get_from_provider() to look up what we need, but that seems kind of kludgy. Maybe a new function could encapsulate the messy details of that. Do you have any suggestions? I can create some new common code if appropriate, but only if it represents missing functionality that's generally useful. -Alex
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)