| Message ID | 20160115103901.1b987f53@bbrezillon (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/15, Boris Brezillon wrote: > Hi Stephen, > > On Thu, 14 Jan 2016 18:02:37 -0800 > Stephen Boyd <sboyd@codeaurora.org> wrote: > > > > Is there any way to get the irq into this probe function without > > getting a clk pointer and then unwrapping it to get an irq > > value out of the clk_hw wrapper structure? That's a pretty > > convoluted design. > > Not sure I get what you're suggesting, but the only solution I see to > avoid this "get main clk pointer dance" would be to have a global > variable storing the clk_main instance (or a list of clk_main > instances). > The thing is, I'd like to avoid adding new global variables in this > clk drivers as much as possible. Moreover, the core is already taking > care of keeping the list of all registered clks, with their associated > of_node, so, is there a real point in creating a duplicated association > list in this driver too? Ok I have to admit I'm a little confused. I thought we were getting the irq from the clkmain structure so that we could request it here in the probe function. But we're really assigning the irq so that we can enable/disable/free it later on? So my new question is if we can register the clocks at the same time as the "platform device" (really a clock) probes. The correct design was probably to have pmc be a big platform device and have it register clocks that it knows exist inside it based on the compatible string. And it would also know which irqs (or enforce some ordering of irqs) to assign to which clocks. It looks like we totally missed that though.... so now we have many DT nodes that are also platform devices. So if we can do it at the same time as irq requests then we're good and the clk_hw structure is in the same scope. If not, then we should probably just let of_clk_hw_get_from_provider() happen (and it needs to happen for other reasons anyway) and call it a day.
On Fri, 15 Jan 2016 17:26:12 -0800 Stephen Boyd <sboyd@codeaurora.org> wrote: > On 01/15, Boris Brezillon wrote: > > Hi Stephen, > > > > On Thu, 14 Jan 2016 18:02:37 -0800 > > Stephen Boyd <sboyd@codeaurora.org> wrote: > > > > > > Is there any way to get the irq into this probe function without > > > getting a clk pointer and then unwrapping it to get an irq > > > value out of the clk_hw wrapper structure? That's a pretty > > > convoluted design. > > > > Not sure I get what you're suggesting, but the only solution I see to > > avoid this "get main clk pointer dance" would be to have a global > > variable storing the clk_main instance (or a list of clk_main > > instances). > > The thing is, I'd like to avoid adding new global variables in this > > clk drivers as much as possible. Moreover, the core is already taking > > care of keeping the list of all registered clks, with their associated > > of_node, so, is there a real point in creating a duplicated association > > list in this driver too? > > Ok I have to admit I'm a little confused. I thought we were > getting the irq from the clkmain structure so that we could > request it here in the probe function. But we're really assigning > the irq so that we can enable/disable/free it later on? > > So my new question is if we can register the clocks at the same > time as the "platform device" (really a clock) probes. The > correct design was probably to have pmc be a big platform device > and have it register clocks that it knows exist inside it based > on the compatible string. And it would also know which irqs (or > enforce some ordering of irqs) to assign to which clocks. > > It looks like we totally missed that though.... so now we have > many DT nodes that are also platform devices. So if we can do it > at the same time as irq requests then we're good and the clk_hw > structure is in the same scope. If not, then we should probably > just let of_clk_hw_get_from_provider() happen (and it needs to > happen for other reasons anyway) and call it a day. > Actually the PMC is not only exposing clocks. You have a bit in one of its registers that is used by a USB driver to enable a BIAS, and probably other things that are not directly related to clocks. Hence the idea to declare the PMC as an MFD/syscon device (which is in turn providing an IRQ chip). Note that the irq problem we're talking about cannot be addressed by simply moving everything into the ->probe() function of the platform driver: we need the clock quite early in the boot process (it's required for the initial clocksource device), in the other hand you can't request threaded irqs when you're so early in the boot process, and this becomes a problem when you apply the preempt-RT patch which moves all standard irqs to threaded ones. So the idea was to use polling instead of the irq event until we are able to register threaded irqs, and then switch to an irq-able approach once everything is in place (platform devices are probed after the thread infrastructure has been setup, which is why we used this trick). This whole series was actually made for 2 reasons: - expose the PMC as a syscon device so that the USB driver can access it's register without having to expose a global void __iomem * variable - address the preemt-RT/threaded irq problem I explained above
On 01/16, Boris Brezillon wrote: > Actually the PMC is not only exposing clocks. You have a bit in one of > its registers that is used by a USB driver to enable a BIAS, and > probably other things that are not directly related to clocks. Hence > the idea to declare the PMC as an MFD/syscon device (which is in turn > providing an IRQ chip). > > Note that the irq problem we're talking about cannot be addressed by > simply moving everything into the ->probe() function of the platform > driver: we need the clock quite early in the boot process (it's > required for the initial clocksource device), in the other hand you > can't request threaded irqs when you're so early in the boot process, > and this becomes a problem when you apply the preempt-RT patch which > moves all standard irqs to threaded ones. > So the idea was to use polling instead of the irq event until we are > able to register threaded irqs, and then switch to an irq-able approach > once everything is in place (platform devices are probed after the > thread infrastructure has been setup, which is why we used this trick). Just one more thought. If we had one platform driver for the entire PMC could we also have an OF_CLK_DECLARE for it as well? Then that OF_CLK_DECLARE could run early, register whatever clocks are essential for the clocksource to work, and defer the rest of the clock registration to when the device driver probes? We'd still need some sort of global list of clocks that we registered early, and this list might need to be per PMC if there's more than one PMC, but we'd have all the clk_hw pointers available when the platform driver probed. Oh and we'd need clk_get() to return PROBE_DEFER too so that consumer drivers could be deferred until the PMC driver probes. I suppose this would be making more global variables, which isn't preferred, so let's worry about this clk_hw vs clk pointer problem later. We'll have to keep this driver in mind when we're converting over clk providers to registering clk_hw pointers with the of clk provider layer. Acked-by: Stephen Boyd <sboyd@codeaurora.org>
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index f13c3f4..fbef1b7 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -3001,6 +3001,42 @@ void of_clk_del_provider(struct device_node *np) } EXPORT_SYMBOL_GPL(of_clk_del_provider); +/** + * of_clk_hw_get_from_provider() - Look for a clk_hw instance registered with + * the given clkspec definition. + * @clkspec: pointer to a clock specifier data structure + * + * This function looks up a struct clk_hw from the registered list of clock + * providers, an input is a clock specifier data structure as returned + * from the of_parse_phandle_with_args() function call. + */ +struct clk_hw *of_clk_hw_get_from_provider(struct of_phandle_args *clkspec) +{ + struct clk_hw *clk_hw = ERR_PTR(-EPROBE_DEFER); + struct of_clk_provider *provider; + + if (!clkspec) + return ERR_PTR(-EINVAL); + + /* Check if we have such a provider in our array */ + mutex_lock(&of_clk_mutex); + list_for_each_entry(provider, &of_clk_providers, link) { + struct clk *clk = ERR_PTR(-EPROBE_DEFER); + + if (provider->node == clkspec->np) + clk = provider->get(clkspec, provider->data); + + if (!IS_ERR(clk)) { + clk_hw = __clk_get_hw(clk); + break; + } + } + mutex_unlock(&of_clk_mutex); + + return clk_hw; +} +EXPORT_SYMBOL_GPL(of_clk_hw_get_from_provider); + struct clk *__of_clk_get_from_provider(struct of_phandle_args *clkspec, const char *dev_id, const char *con_id) { diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h index c56988a..bb62517 100644 --- a/include/linux/clk-provider.h +++ b/include/linux/clk-provider.h @@ -645,6 +645,7 @@ void devm_clk_unregister(struct device *dev, struct clk *clk); const char *__clk_get_name(const struct clk *clk); const char *clk_hw_get_name(const struct clk_hw *hw); struct clk_hw *__clk_get_hw(struct clk *clk); +struct clk_hw *of_clk_hw_get_from_provider(struct of_phandle_args *clkspec); unsigned int clk_hw_get_num_parents(const struct clk_hw *hw); struct clk_hw *clk_hw_get_parent(const struct clk_hw *hw); struct clk_hw *clk_hw_get_parent_by_index(const struct clk_hw *hw,