| Message ID | 20170921164940.20343-1-georgi.djakov@linaro.org |
|---|---|
| Headers | show |
| Series | Add support for Qualcomm A53 CPU clock | expand |
On 09/21/2017 07:49 PM, Georgi Djakov wrote: > This patchset adds support for the A53 CPU clock on MSM8916 platforms > and allows scaling of the CPU frequency on msm8916 based platforms. > > Changes since v8 (https://lkml.org/lkml/2017/6/23/476) > * Converted APCS mailbox driver to use regmap and to populate child > platform devices that will handle the rest of the functionality > provided by APCS block. > * Picked Rob's Ack for the PLL binding. > * Changed the APCS binding and put it into a separate patch. > * Addressed review comments. > * Minor changes. Hi Stephen and Bjorn. A gentle ping on this.. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 21 Sep 09:49 PDT 2017, Georgi Djakov wrote: > Move the structure shared by the APCS IPC device and its subdevices > into a separate header file. > As you're creating the apcs regmap with devm_regmap_init_mmio() you can just call dev_get_regmap(dev->parent) in your child to get the handle. But I would prefer that you just add the clock code to the existing driver. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 21 Sep 09:49 PDT 2017, Georgi Djakov wrote: > Add a driver for the APCS clock controller. It is part of the APCS > hardware block, which among other things implements also a combined > mux and half integer divider functionality. It can choose between a > fixed-rate clock or the dedicated APCS (A53) PLL. The source and the > divider can be set both at the same time. > > This is required for enabling CPU frequency scaling on MSM8916-based > platforms. As stated in the binding patch I think you should describe the "two" parts in one node and probably add this code to the existing driver, rather than spawning a new device. [..] > diff --git a/drivers/clk/qcom/apcs-msm8916.c b/drivers/clk/qcom/apcs-msm8916.c [..] > +#include "clk-regmap.h" > +#include "clk-regmap-mux-div.h" These two include files might cause some issues, but I would prefer that you bake this code into the existing apcs driver. [..] > +static int __init qcom_apcs_msm8916_clk_init(void) > +{ > + return platform_driver_register(&qcom_apcs_msm8916_clk_driver); > +} > +core_initcall(qcom_apcs_msm8916_clk_init); NB. The a53 clock is a builtin_platform_driver(), i.e. device_initcall() the clock will never be available at core_initcall(), so the devm_clk_get() should always hit a probe defer. Use module_platform_driver() instead. > + > +static void __exit qcom_apcs_msm8916_clk_exit(void) > +{ > + platform_driver_unregister(&qcom_apcs_msm8916_clk_driver); > +} > +module_exit(qcom_apcs_msm8916_clk_exit); Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 21 Sep 09:49 PDT 2017, Georgi Djakov wrote: > This hardware block provides more functionalities that just IPC. Convert > it to regmap to allow other child platform devices to use the same regmap. > Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Bjorn, Thanks for reviewing! On 10/26/2017 07:28 AM, Bjorn Andersson wrote: > On Thu 21 Sep 09:49 PDT 2017, Georgi Djakov wrote: > >> Move the structure shared by the APCS IPC device and its subdevices >> into a separate header file. >> > > As you're creating the apcs regmap with devm_regmap_init_mmio() you can > just call dev_get_regmap(dev->parent) in your child to get the handle. Ok, thanks! > > But I would prefer that you just add the clock code to the existing > driver. This will require an ack from Stephen, and i got the impression that he prefers a separate clk driver [1]. Stephen, are you ok with registering the clocks from the apcs mailbox driver? [1] https://lkml.org/lkml/2017/6/26/750 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 10/27, Georgi Djakov wrote: > Hi Bjorn, > > Thanks for reviewing! > > On 10/26/2017 07:28 AM, Bjorn Andersson wrote: > > On Thu 21 Sep 09:49 PDT 2017, Georgi Djakov wrote: > > > >> Move the structure shared by the APCS IPC device and its subdevices > >> into a separate header file. > >> > > > > As you're creating the apcs regmap with devm_regmap_init_mmio() you can > > just call dev_get_regmap(dev->parent) in your child to get the handle. > > Ok, thanks! > > > > > But I would prefer that you just add the clock code to the existing > > driver. > > This will require an ack from Stephen, and i got the impression that he > prefers a separate clk driver [1]. > > Stephen, are you ok with registering the clocks from the apcs mailbox > driver? > > [1] https://lkml.org/lkml/2017/6/26/750 The parent regmap "trick" was the plan. Is something wrong with that? Not having random clk drivers scattered throughout the tree is sort of nice because it makes for an easier time finding things that are similar. Maybe that's an abuse of the driver model though? Just to get things into some same directory. I'm fine either way.
On Mon 13 Nov 18:12 PST 2017, Stephen Boyd wrote: > On 10/27, Georgi Djakov wrote: > > Hi Bjorn, > > > > Thanks for reviewing! > > > > On 10/26/2017 07:28 AM, Bjorn Andersson wrote: > > > On Thu 21 Sep 09:49 PDT 2017, Georgi Djakov wrote: > > > > > >> Move the structure shared by the APCS IPC device and its subdevices > > >> into a separate header file. > > >> > > > > > > As you're creating the apcs regmap with devm_regmap_init_mmio() you can > > > just call dev_get_regmap(dev->parent) in your child to get the handle. > > > > Ok, thanks! > > > > > > > > But I would prefer that you just add the clock code to the existing > > > driver. > > > > This will require an ack from Stephen, and i got the impression that he > > prefers a separate clk driver [1]. > > > > Stephen, are you ok with registering the clocks from the apcs mailbox > > driver? > > > > [1] https://lkml.org/lkml/2017/6/26/750 > > The parent regmap "trick" was the plan. Is something wrong with > that? > Not at all, but then this patch (moving apcs context to a shared header file) shouldn't be needed, or am I missing something? > Not having random clk drivers scattered throughout the tree is > sort of nice because it makes for an easier time finding things > that are similar. Maybe that's an abuse of the driver model > though? Just to get things into some same directory. I'm fine > either way. > Keeping the clock driver in the clock subsystem does make sense. I see now that there is a include of a local header file as well, so that would just be messy to keep split. I'm fine with the extra driver instance, it's the DT that I don't think should describe the fact that we want to keep the clock-part in the clock subsystem. Do you see any problems spawning the clock driver programmatically and then calling of_clk_add_hw_provider() on the parent's of_node? Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/13, Bjorn Andersson wrote: > On Mon 13 Nov 18:12 PST 2017, Stephen Boyd wrote: > > > On 10/27, Georgi Djakov wrote: > > > Hi Bjorn, > > > > > > Thanks for reviewing! > > > > > > On 10/26/2017 07:28 AM, Bjorn Andersson wrote: > > > > On Thu 21 Sep 09:49 PDT 2017, Georgi Djakov wrote: > > > > > > > >> Move the structure shared by the APCS IPC device and its subdevices > > > >> into a separate header file. > > > >> > > > > > > > > As you're creating the apcs regmap with devm_regmap_init_mmio() you can > > > > just call dev_get_regmap(dev->parent) in your child to get the handle. > > > > > > Ok, thanks! > > > > > > > > > > > But I would prefer that you just add the clock code to the existing > > > > driver. > > > > > > This will require an ack from Stephen, and i got the impression that he > > > prefers a separate clk driver [1]. > > > > > > Stephen, are you ok with registering the clocks from the apcs mailbox > > > driver? > > > > > > [1] https://lkml.org/lkml/2017/6/26/750 > > > > The parent regmap "trick" was the plan. Is something wrong with > > that? > > > > Not at all, but then this patch (moving apcs context to a shared header > file) shouldn't be needed, or am I missing something? Agreed. > > > Not having random clk drivers scattered throughout the tree is > > sort of nice because it makes for an easier time finding things > > that are similar. Maybe that's an abuse of the driver model > > though? Just to get things into some same directory. I'm fine > > either way. > > > > Keeping the clock driver in the clock subsystem does make sense. I see > now that there is a include of a local header file as well, so that > would just be messy to keep split. > > I'm fine with the extra driver instance, it's the DT that I don't think > should describe the fact that we want to keep the clock-part in the > clock subsystem. > > Do you see any problems spawning the clock driver programmatically and > then calling of_clk_add_hw_provider() on the parent's of_node? Nope. We shouldn't need to modify DT to make this work.