| Message ID | 20161019132816.31073-4-georgi.djakov@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Stephen Boyd |
| Headers | show |
On 10/19, Georgi Djakov wrote: > Add a driver for the A53 Clock Controller. It is a hardware block that > implements a combined mux and half integer divider functionality. It can > choose between a fixed-rate clock or the dedicated A53 PLL. The source > and the divider can be set both at the same time. > > This is required for enabling CPU frequency scaling on platforms like > MSM8916. > Please Cc DT reviewers. > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> > --- > .../devicetree/bindings/clock/qcom,a53cc.txt | 22 +++ > drivers/clk/qcom/Kconfig | 8 ++ > drivers/clk/qcom/Makefile | 1 + > drivers/clk/qcom/a53cc.c | 155 +++++++++++++++++++++ > 4 files changed, 186 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt > create mode 100644 drivers/clk/qcom/a53cc.c > > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > new file mode 100644 > index 000000000000..a025f062f177 > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > @@ -0,0 +1,22 @@ > +Qualcomm A53 CPU Clock Controller Binding > +------------------------------------------------ > +The A53 CPU Clock Controller is hardware, which provides a combined > +mux and divider functionality for the CPU clocks. It can choose between > +a fixed rate clock and the dedicated A53 PLL. > + > +Required properties : > +- compatible : shall contain: > + > + "qcom,a53cc" > + > +- reg : shall contain base register location and length > + of the APCS region > +- #clock-cells : shall contain 1 > + > +Example: > + > + apcs: syscon@b011000 { > + compatible = "qcom,a53cc", "syscon"; Why is it a syscon? Is that part used? > + reg = <0x0b011000 0x1000>; > + #clock-cells = <1>; > + }; > diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig > index a889f0b14b54..59dfcdc340e4 100644 > --- a/drivers/clk/qcom/Kconfig > +++ b/drivers/clk/qcom/Kconfig > @@ -159,3 +159,11 @@ config QCOM_A53PLL > support for CPU frequencies above 1GHz. > Say Y if you want to support CPU frequency scaling on devices > such as MSM8916. > + > +config QCOM_A53CC > + bool "A53 Clock Controller" Can't these configs be tristate? Same applies to A53PLL. > + depends on COMMON_CLK_QCOM && QCOM_A53PLL > + help > + Support for the A53 clock controller on some Qualcomm devices. > + Say Y if you want to support CPU frequency scaling on devices > + such as MSM8916. > diff --git a/drivers/clk/qcom/a53cc.c b/drivers/clk/qcom/a53cc.c > new file mode 100644 > index 000000000000..4d20db9da407 > --- /dev/null > +++ b/drivers/clk/qcom/a53cc.c > @@ -0,0 +1,155 @@ > +/* > + * Copyright (c) 2016, Linaro Limited > + * Copyright (c) 2014, The Linux Foundation. All rights reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 and > + * only version 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/clk.h> > +#include <linux/clk-provider.h> > +#include <linux/cpu.h> Is this include used? > +#include <linux/kernel.h> > +#include <linux/platform_device.h> > +#include <linux/regmap.h> > + > +#include "clk-regmap.h" > +#include "clk-regmap-mux-div.h" > + > +enum { > + P_GPLL0, > + P_A53PLL, > +}; > + > +static const struct parent_map gpll0_a53cc_map[] = { > + { P_GPLL0, 4 }, > + { P_A53PLL, 5 }, > +}; > + > +static const char * const gpll0_a53cc[] = { > + "gpll0_vote", > + "a53pll", > +}; > + > +static const struct regmap_config a53cc_regmap_config = { > + .reg_bits = 32, > + .reg_stride = 4, > + .val_bits = 32, > + .max_register = 0x1000, > + .fast_io = true, > + .val_format_endian = REGMAP_ENDIAN_LITTLE, > +}; > + > +static const struct of_device_id qcom_a53cc_match_table[] = { > + { .compatible = "qcom,a53cc" }, > + { } > +}; Can you move this down next to the driver please? > + > +/* > + * We use the notifier function for switching to a temporary safe configuration > + * (mux and divider), while the a53 pll is reconfigured. > + */ > +static int a53cc_notifier_cb(struct notifier_block *nb, unsigned long event, > + void *data) > +{ > + int ret = 0; > + struct clk_regmap_mux_div *md = container_of(nb, > + struct clk_regmap_mux_div, > + clk_nb); > + > + if (event == PRE_RATE_CHANGE) > + ret = __mux_div_set_src_div(md, md->safe_src, md->safe_div); > + > + return notifier_from_errno(ret); > +} > + > +static int qcom_a53cc_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct clk_regmap_mux_div *a53cc; > + struct resource *res; > + void __iomem *base; > + struct clk *pclk; > + struct regmap *regmap; > + struct clk_init_data init; = { } for safety? > + int ret; > + > + a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL); > + if (!a53cc) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + a53cc->reg_offset = 0x50, > + a53cc->hid_width = 5, > + a53cc->hid_shift = 0, > + a53cc->src_width = 3, > + a53cc->src_shift = 8, > + a53cc->safe_src = 4, > + a53cc->safe_div = 3, Replace commas with semicolons please. Also do we need the safe things to be part of the struct? The notifier is here so we could just as easily hard code these things in the notifier. > + a53cc->parent_map = gpll0_a53cc_map, > + > + init.name = "a53mux", > + init.parent_names = gpll0_a53cc, > + init.num_parents = 2, > + init.ops = &clk_regmap_mux_div_ops, > + init.flags = CLK_SET_RATE_PARENT; > + a53cc->clkr.hw.init = &init; > + > + pclk = __clk_lookup(gpll0_a53cc[1]); > + if (!pclk) > + return -EPROBE_DEFER; > + > + a53cc->clk_nb.notifier_call = a53cc_notifier_cb; > + ret = clk_notifier_register(pclk, &a53cc->clk_nb); > + if (ret) { > + dev_err(dev, "failed to register clock notifier: %d\n", ret); > + return ret; > + }
On 10/28/2016 04:54 AM, Stephen Boyd wrote: > On 10/19, Georgi Djakov wrote: >> Add a driver for the A53 Clock Controller. It is a hardware block that >> implements a combined mux and half integer divider functionality. It can >> choose between a fixed-rate clock or the dedicated A53 PLL. The source >> and the divider can be set both at the same time. >> >> This is required for enabling CPU frequency scaling on platforms like >> MSM8916. >> > > Please Cc DT reviewers. > Ok, will do. >> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> >> --- >> .../devicetree/bindings/clock/qcom,a53cc.txt | 22 +++ >> drivers/clk/qcom/Kconfig | 8 ++ >> drivers/clk/qcom/Makefile | 1 + >> drivers/clk/qcom/a53cc.c | 155 +++++++++++++++++++++ >> 4 files changed, 186 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt >> create mode 100644 drivers/clk/qcom/a53cc.c >> >> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt >> new file mode 100644 >> index 000000000000..a025f062f177 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt >> @@ -0,0 +1,22 @@ >> +Qualcomm A53 CPU Clock Controller Binding >> +------------------------------------------------ >> +The A53 CPU Clock Controller is hardware, which provides a combined >> +mux and divider functionality for the CPU clocks. It can choose between >> +a fixed rate clock and the dedicated A53 PLL. >> + >> +Required properties : >> +- compatible : shall contain: >> + >> + "qcom,a53cc" >> + >> +- reg : shall contain base register location and length >> + of the APCS region >> +- #clock-cells : shall contain 1 >> + >> +Example: >> + >> + apcs: syscon@b011000 { >> + compatible = "qcom,a53cc", "syscon"; > > Why is it a syscon? Is that part used? It is not used in this patchset. I will remove the syscon part from the example and will change the compatible to qcom,a53cc-msm8916, as this also seems to be SoC specific. > >> + reg = <0x0b011000 0x1000>; >> + #clock-cells = <1>; >> + }; >> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig >> index a889f0b14b54..59dfcdc340e4 100644 >> --- a/drivers/clk/qcom/Kconfig >> +++ b/drivers/clk/qcom/Kconfig >> @@ -159,3 +159,11 @@ config QCOM_A53PLL >> support for CPU frequencies above 1GHz. >> Say Y if you want to support CPU frequency scaling on devices >> such as MSM8916. >> + >> +config QCOM_A53CC >> + bool "A53 Clock Controller" > > Can't these configs be tristate? Same applies to A53PLL. > I am using __clk_lookup() in this patch below, which is not exported. The A53PLL could be a tristate, but i just made them both builtins for consistency. >> + depends on COMMON_CLK_QCOM && QCOM_A53PLL >> + help >> + Support for the A53 clock controller on some Qualcomm devices. >> + Say Y if you want to support CPU frequency scaling on devices >> + such as MSM8916. >> diff --git a/drivers/clk/qcom/a53cc.c b/drivers/clk/qcom/a53cc.c >> new file mode 100644 >> index 000000000000..4d20db9da407 >> --- /dev/null >> +++ b/drivers/clk/qcom/a53cc.c >> @@ -0,0 +1,155 @@ >> +/* >> + * Copyright (c) 2016, Linaro Limited >> + * Copyright (c) 2014, The Linux Foundation. All rights reserved. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 and >> + * only version 2 as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/clk-provider.h> >> +#include <linux/cpu.h> > > Is this include used? It isn't. Removed! > >> +#include <linux/kernel.h> >> +#include <linux/platform_device.h> >> +#include <linux/regmap.h> >> + >> +#include "clk-regmap.h" >> +#include "clk-regmap-mux-div.h" >> + >> +enum { >> + P_GPLL0, >> + P_A53PLL, >> +}; >> + >> +static const struct parent_map gpll0_a53cc_map[] = { >> + { P_GPLL0, 4 }, >> + { P_A53PLL, 5 }, >> +}; >> + >> +static const char * const gpll0_a53cc[] = { >> + "gpll0_vote", >> + "a53pll", >> +}; >> + >> +static const struct regmap_config a53cc_regmap_config = { >> + .reg_bits = 32, >> + .reg_stride = 4, >> + .val_bits = 32, >> + .max_register = 0x1000, >> + .fast_io = true, >> + .val_format_endian = REGMAP_ENDIAN_LITTLE, >> +}; >> + >> +static const struct of_device_id qcom_a53cc_match_table[] = { >> + { .compatible = "qcom,a53cc" }, >> + { } >> +}; > > Can you move this down next to the driver please? > Ok, sure. >> + >> +/* >> + * We use the notifier function for switching to a temporary safe configuration >> + * (mux and divider), while the a53 pll is reconfigured. >> + */ >> +static int a53cc_notifier_cb(struct notifier_block *nb, unsigned long event, >> + void *data) >> +{ >> + int ret = 0; >> + struct clk_regmap_mux_div *md = container_of(nb, >> + struct clk_regmap_mux_div, >> + clk_nb); >> + >> + if (event == PRE_RATE_CHANGE) >> + ret = __mux_div_set_src_div(md, md->safe_src, md->safe_div); >> + >> + return notifier_from_errno(ret); >> +} >> + >> +static int qcom_a53cc_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct clk_regmap_mux_div *a53cc; >> + struct resource *res; >> + void __iomem *base; >> + struct clk *pclk; >> + struct regmap *regmap; >> + struct clk_init_data init; > > = { } for safety? > Yes, thanks! >> + int ret; >> + >> + a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL); >> + if (!a53cc) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + a53cc->reg_offset = 0x50, >> + a53cc->hid_width = 5, >> + a53cc->hid_shift = 0, >> + a53cc->src_width = 3, >> + a53cc->src_shift = 8, >> + a53cc->safe_src = 4, >> + a53cc->safe_div = 3, > > Replace commas with semicolons please. > > Also do we need the safe things to be part of the struct? The > notifier is here so we could just as easily hard code these > things in the notifier. > Ok, will hardcode it with some comment. Thanks again for reviewing! BR, Georgi >> + a53cc->parent_map = gpll0_a53cc_map, >> + >> + init.name = "a53mux", >> + init.parent_names = gpll0_a53cc, >> + init.num_parents = 2, >> + init.ops = &clk_regmap_mux_div_ops, >> + init.flags = CLK_SET_RATE_PARENT; >> + a53cc->clkr.hw.init = &init; >> + >> + pclk = __clk_lookup(gpll0_a53cc[1]); >> + if (!pclk) >> + return -EPROBE_DEFER; >> + >> + a53cc->clk_nb.notifier_call = a53cc_notifier_cb; >> + ret = clk_notifier_register(pclk, &a53cc->clk_nb); >> + if (ret) { >> + dev_err(dev, "failed to register clock notifier: %d\n", ret); >> + return ret; >> + } > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote: > On 10/19, Georgi Djakov wrote: > > Add a driver for the A53 Clock Controller. It is a hardware block that > > implements a combined mux and half integer divider functionality. It can > > choose between a fixed-rate clock or the dedicated A53 PLL. The source > > and the divider can be set both at the same time. > > > > This is required for enabling CPU frequency scaling on platforms like > > MSM8916. > > > > Please Cc DT reviewers. > > > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> > > --- > > .../devicetree/bindings/clock/qcom,a53cc.txt | 22 +++ > > drivers/clk/qcom/Kconfig | 8 ++ > > drivers/clk/qcom/Makefile | 1 + > > drivers/clk/qcom/a53cc.c | 155 +++++++++++++++++++++ > > 4 files changed, 186 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt > > create mode 100644 drivers/clk/qcom/a53cc.c > > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > > new file mode 100644 > > index 000000000000..a025f062f177 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > > @@ -0,0 +1,22 @@ > > +Qualcomm A53 CPU Clock Controller Binding > > +------------------------------------------------ > > +The A53 CPU Clock Controller is hardware, which provides a combined > > +mux and divider functionality for the CPU clocks. It can choose between > > +a fixed rate clock and the dedicated A53 PLL. > > + > > +Required properties : > > +- compatible : shall contain: > > + > > + "qcom,a53cc" > > + > > +- reg : shall contain base register location and length > > + of the APCS region > > +- #clock-cells : shall contain 1 > > + > > +Example: > > + > > + apcs: syscon@b011000 { > > + compatible = "qcom,a53cc", "syscon"; > > Why is it a syscon? Is that part used? > I use the register at offset 8 for interrupting the other subsystems, so this must be available as something I can poke. Which makes me think that this should be described as a "simple-mfd" and "syscon" with the a53cc node as a child - grabbing the regmap of the syscon parent, rather then ioremapping the same region again. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/02, Bjorn Andersson wrote: > On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote: > > > On 10/19, Georgi Djakov wrote: > > > Add a driver for the A53 Clock Controller. It is a hardware block that > > > implements a combined mux and half integer divider functionality. It can > > > choose between a fixed-rate clock or the dedicated A53 PLL. The source > > > and the divider can be set both at the same time. > > > > > > This is required for enabling CPU frequency scaling on platforms like > > > MSM8916. > > > > > > > Please Cc DT reviewers. > > > > > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> > > > --- > > > .../devicetree/bindings/clock/qcom,a53cc.txt | 22 +++ > > > drivers/clk/qcom/Kconfig | 8 ++ > > > drivers/clk/qcom/Makefile | 1 + > > > drivers/clk/qcom/a53cc.c | 155 +++++++++++++++++++++ > > > 4 files changed, 186 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt > > > create mode 100644 drivers/clk/qcom/a53cc.c > > > > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > > > new file mode 100644 > > > index 000000000000..a025f062f177 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > > > @@ -0,0 +1,22 @@ > > > +Qualcomm A53 CPU Clock Controller Binding > > > +------------------------------------------------ > > > +The A53 CPU Clock Controller is hardware, which provides a combined > > > +mux and divider functionality for the CPU clocks. It can choose between > > > +a fixed rate clock and the dedicated A53 PLL. > > > + > > > +Required properties : > > > +- compatible : shall contain: > > > + > > > + "qcom,a53cc" > > > + > > > +- reg : shall contain base register location and length > > > + of the APCS region > > > +- #clock-cells : shall contain 1 > > > + > > > +Example: > > > + > > > + apcs: syscon@b011000 { > > > + compatible = "qcom,a53cc", "syscon"; > > > > Why is it a syscon? Is that part used? > > > > I use the register at offset 8 for interrupting the other subsystems, so > this must be available as something I can poke. > > Which makes me think that this should be described as a "simple-mfd" and > "syscon" with the a53cc node as a child - grabbing the regmap of the > syscon parent, rather then ioremapping the same region again. > That's sort of a question for DT reviewers. The register space certainly seems like a free for all with a tilt toward power management of the CPU, similar to how this was done on Krait based designs. I wonder why we didn't make up some provider/consumer binding for the "kicking" feature used by SMD/RPM code. Then this could be a clock provider and a "kick" provider (haha #kick-cells) and the usage of syscon/regmap wouldn't be mandatory.
On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote: > On 11/02, Bjorn Andersson wrote: > > On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote: > > > > > On 10/19, Georgi Djakov wrote: > > > > Add a driver for the A53 Clock Controller. It is a hardware block that > > > > implements a combined mux and half integer divider functionality. It can > > > > choose between a fixed-rate clock or the dedicated A53 PLL. The source > > > > and the divider can be set both at the same time. > > > > > > > > This is required for enabling CPU frequency scaling on platforms like > > > > MSM8916. > > > > > > > > > > Please Cc DT reviewers. > > > > > > > Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> > > > > --- > > > > .../devicetree/bindings/clock/qcom,a53cc.txt | 22 +++ > > > > drivers/clk/qcom/Kconfig | 8 ++ > > > > drivers/clk/qcom/Makefile | 1 + > > > > drivers/clk/qcom/a53cc.c | 155 +++++++++++++++++++++ > > > > 4 files changed, 186 insertions(+) > > > > create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt > > > > create mode 100644 drivers/clk/qcom/a53cc.c > > > > > > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > > > > new file mode 100644 > > > > index 000000000000..a025f062f177 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > > > > @@ -0,0 +1,22 @@ > > > > +Qualcomm A53 CPU Clock Controller Binding > > > > +------------------------------------------------ > > > > +The A53 CPU Clock Controller is hardware, which provides a combined > > > > +mux and divider functionality for the CPU clocks. It can choose between > > > > +a fixed rate clock and the dedicated A53 PLL. > > > > + > > > > +Required properties : > > > > +- compatible : shall contain: > > > > + > > > > + "qcom,a53cc" > > > > + > > > > +- reg : shall contain base register location and length > > > > + of the APCS region > > > > +- #clock-cells : shall contain 1 > > > > + > > > > +Example: > > > > + > > > > + apcs: syscon@b011000 { > > > > + compatible = "qcom,a53cc", "syscon"; > > > > > > Why is it a syscon? Is that part used? > > > > > > > I use the register at offset 8 for interrupting the other subsystems, so > > this must be available as something I can poke. > > > > Which makes me think that this should be described as a "simple-mfd" and > > "syscon" with the a53cc node as a child - grabbing the regmap of the > > syscon parent, rather then ioremapping the same region again. > > > > That's sort of a question for DT reviewers. The register space > certainly seems like a free for all with a tilt toward power > management of the CPU, similar to how this was done on Krait > based designs. > Right. But this kind of mashup blocks was the reason why simple-mfd was put in place. > I wonder why we didn't make up some provider/consumer binding for > the "kicking" feature used by SMD/RPM code. Then this could be a > clock provider and a "kick" provider (haha #kick-cells) and the > usage of syscon/regmap wouldn't be mandatory. > I did consider doing that, but had enough dependencies to put in place as it was. I'm in favour of us inventing a kicker API and it's found outside out use cases as well (e.g. virtio/rpmsg). Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/03/2016 08:28 PM, Bjorn Andersson wrote: > On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote: > >> On 11/02, Bjorn Andersson wrote: >>> On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote: >>> >>>> On 10/19, Georgi Djakov wrote: >>>>> Add a driver for the A53 Clock Controller. It is a hardware block that >>>>> implements a combined mux and half integer divider functionality. It can >>>>> choose between a fixed-rate clock or the dedicated A53 PLL. The source >>>>> and the divider can be set both at the same time. >>>>> >>>>> This is required for enabling CPU frequency scaling on platforms like >>>>> MSM8916. >>>>> >>>> >>>> Please Cc DT reviewers. >>>> >>>>> Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> >>>>> --- >>>>> .../devicetree/bindings/clock/qcom,a53cc.txt | 22 +++ >>>>> drivers/clk/qcom/Kconfig | 8 ++ >>>>> drivers/clk/qcom/Makefile | 1 + >>>>> drivers/clk/qcom/a53cc.c | 155 +++++++++++++++++++++ >>>>> 4 files changed, 186 insertions(+) >>>>> create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt >>>>> create mode 100644 drivers/clk/qcom/a53cc.c >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt >>>>> new file mode 100644 >>>>> index 000000000000..a025f062f177 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt >>>>> @@ -0,0 +1,22 @@ >>>>> +Qualcomm A53 CPU Clock Controller Binding >>>>> +------------------------------------------------ >>>>> +The A53 CPU Clock Controller is hardware, which provides a combined >>>>> +mux and divider functionality for the CPU clocks. It can choose between >>>>> +a fixed rate clock and the dedicated A53 PLL. >>>>> + >>>>> +Required properties : >>>>> +- compatible : shall contain: >>>>> + >>>>> + "qcom,a53cc" >>>>> + >>>>> +- reg : shall contain base register location and length >>>>> + of the APCS region >>>>> +- #clock-cells : shall contain 1 >>>>> + >>>>> +Example: >>>>> + >>>>> + apcs: syscon@b011000 { >>>>> + compatible = "qcom,a53cc", "syscon"; >>>> >>>> Why is it a syscon? Is that part used? >>>> >>> >>> I use the register at offset 8 for interrupting the other subsystems, so >>> this must be available as something I can poke. >>> >>> Which makes me think that this should be described as a "simple-mfd" and >>> "syscon" with the a53cc node as a child - grabbing the regmap of the >>> syscon parent, rather then ioremapping the same region again. >>> >> >> That's sort of a question for DT reviewers. The register space >> certainly seems like a free for all with a tilt toward power >> management of the CPU, similar to how this was done on Krait >> based designs. >> > > Right. But this kind of mashup blocks was the reason why simple-mfd was > put in place. > Ok, thanks for the comments. Then i will make it look like this: apcs: syscon@b011000 { compatible = "syscon", "simple-mfd"; reg = <0x0b011000 0x1000>; a53mux: clock { compatible = "qcom,msm8916-a53cc"; #clock-cells = <1>; }; }; Thanks, Georgi >> I wonder why we didn't make up some provider/consumer binding for >> the "kicking" feature used by SMD/RPM code. Then this could be a >> clock provider and a "kick" provider (haha #kick-cells) and the >> usage of syscon/regmap wouldn't be mandatory. >> > > I did consider doing that, but had enough dependencies to put in place > as it was. > > I'm in favour of us inventing a kicker API and it's found outside out > use cases as well (e.g. virtio/rpmsg). > > Regards, > Bjorn > -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/11, Georgi Djakov wrote: > On 11/03/2016 08:28 PM, Bjorn Andersson wrote: > >On Wed 02 Nov 15:55 PDT 2016, Stephen Boyd wrote: > > > >>On 11/02, Bjorn Andersson wrote: > >>>On Thu 27 Oct 18:54 PDT 2016, Stephen Boyd wrote: > >>> > >>>>On 10/19, Georgi Djakov wrote: > >>>>>Add a driver for the A53 Clock Controller. It is a hardware block that > >>>>>implements a combined mux and half integer divider functionality. It can > >>>>>choose between a fixed-rate clock or the dedicated A53 PLL. The source > >>>>>and the divider can be set both at the same time. > >>>>> > >>>>>This is required for enabling CPU frequency scaling on platforms like > >>>>>MSM8916. > >>>>> > >>>> > >>>>Please Cc DT reviewers. > >>>> > >>>>>Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> > >>>>>--- > >>>>> .../devicetree/bindings/clock/qcom,a53cc.txt | 22 +++ > >>>>> drivers/clk/qcom/Kconfig | 8 ++ > >>>>> drivers/clk/qcom/Makefile | 1 + > >>>>> drivers/clk/qcom/a53cc.c | 155 +++++++++++++++++++++ > >>>>> 4 files changed, 186 insertions(+) > >>>>> create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt > >>>>> create mode 100644 drivers/clk/qcom/a53cc.c > >>>>> > >>>>>diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > >>>>>new file mode 100644 > >>>>>index 000000000000..a025f062f177 > >>>>>--- /dev/null > >>>>>+++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt > >>>>>@@ -0,0 +1,22 @@ > >>>>>+Qualcomm A53 CPU Clock Controller Binding > >>>>>+------------------------------------------------ > >>>>>+The A53 CPU Clock Controller is hardware, which provides a combined > >>>>>+mux and divider functionality for the CPU clocks. It can choose between > >>>>>+a fixed rate clock and the dedicated A53 PLL. > >>>>>+ > >>>>>+Required properties : > >>>>>+- compatible : shall contain: > >>>>>+ > >>>>>+ "qcom,a53cc" > >>>>>+ > >>>>>+- reg : shall contain base register location and length > >>>>>+ of the APCS region > >>>>>+- #clock-cells : shall contain 1 > >>>>>+ > >>>>>+Example: > >>>>>+ > >>>>>+ apcs: syscon@b011000 { > >>>>>+ compatible = "qcom,a53cc", "syscon"; > >>>> > >>>>Why is it a syscon? Is that part used? > >>>> > >>> > >>>I use the register at offset 8 for interrupting the other subsystems, so > >>>this must be available as something I can poke. > >>> > >>>Which makes me think that this should be described as a "simple-mfd" and > >>>"syscon" with the a53cc node as a child - grabbing the regmap of the > >>>syscon parent, rather then ioremapping the same region again. > >>> > >> > >>That's sort of a question for DT reviewers. The register space > >>certainly seems like a free for all with a tilt toward power > >>management of the CPU, similar to how this was done on Krait > >>based designs. > >> > > > >Right. But this kind of mashup blocks was the reason why simple-mfd was > >put in place. > > > > Ok, thanks for the comments. Then i will make it look like this: > > apcs: syscon@b011000 { > compatible = "syscon", "simple-mfd"; > reg = <0x0b011000 0x1000>; > > a53mux: clock { > compatible = "qcom,msm8916-a53cc"; > #clock-cells = <1>; > }; > }; > > Thanks, > Georgi > > >>I wonder why we didn't make up some provider/consumer binding for > >>the "kicking" feature used by SMD/RPM code. Then this could be a > >>clock provider and a "kick" provider (haha #kick-cells) and the > >>usage of syscon/regmap wouldn't be mandatory. > >> > > > >I did consider doing that, but had enough dependencies to put in place > >as it was. > > > >I'm in favour of us inventing a kicker API and it's found outside out > >use cases as well (e.g. virtio/rpmsg). > > I'd rather we did this kicker API as well. That way we don't need to make a syscon and a simple-mfd to get software to work properly. Don't other silicon vendors need a kicker API as well? How are they kicking remote processors in other places? GPIOs?
On Mon 14 Nov 14:21 PST 2016, Stephen Boyd wrote: > On 11/11, Georgi Djakov wrote: > > On 11/03/2016 08:28 PM, Bjorn Andersson wrote: [..] > > >I'm in favour of us inventing a kicker API and it's found outside out > > >use cases as well (e.g. virtio/rpmsg). > > > > > I'd rather we did this kicker API as well. That way we don't need > to make a syscon and a simple-mfd to get software to work > properly. Don't other silicon vendors need a kicker API as well? > How are they kicking remote processors in other places? GPIOs? > In remoteproc I have two of these: 1) da8xx_remoteproc ioremaps a register and writes a bit in it (looks similar to the downstream Qualcomm way) 2) omap_remoteproc acquires a mbox channel, in which it writes a virtqueue id to kick the remote. So one of the two cases could have used such mechanism. We could write up a Qualcomm specific "kicker" and probe the mailing list regarding the interest in making that generic (i.e. changing the names in the API and DT binding). The sucky part is that I believe we have most of our kickers in place already so rpm, smd, smp2p, smsm etc would all need to support both mechanisms. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/05/2016 11:26 PM, Bjorn Andersson wrote: > On Mon 14 Nov 14:21 PST 2016, Stephen Boyd wrote: > >> On 11/11, Georgi Djakov wrote: >>> On 11/03/2016 08:28 PM, Bjorn Andersson wrote: > [..] >>>> I'm in favour of us inventing a kicker API and it's found outside out >>>> use cases as well (e.g. virtio/rpmsg). >>>> >> >> I'd rather we did this kicker API as well. That way we don't need >> to make a syscon and a simple-mfd to get software to work >> properly. Don't other silicon vendors need a kicker API as well? >> How are they kicking remote processors in other places? GPIOs? >> > > In remoteproc I have two of these: > 1) da8xx_remoteproc ioremaps a register and writes a bit in it (looks > similar to the downstream Qualcomm way) > > 2) omap_remoteproc acquires a mbox channel, in which it writes a > virtqueue id to kick the remote. > > So one of the two cases could have used such mechanism. > I also see the potential users of such API mostly in the remoteproc/rpmgs subsystems. > We could write up a Qualcomm specific "kicker" and probe the mailing > list regarding the interest in making that generic (i.e. changing the > names in the API and DT binding). Yes, i am planing to do this. > > The sucky part is that I believe we have most of our kickers in place > already so rpm, smd, smp2p, smsm etc would all need to support both > mechanisms. Agree.. we have to keep compatibility with existing dtbs. Thanks, Georgi -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/clock/qcom,a53cc.txt b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt new file mode 100644 index 000000000000..a025f062f177 --- /dev/null +++ b/Documentation/devicetree/bindings/clock/qcom,a53cc.txt @@ -0,0 +1,22 @@ +Qualcomm A53 CPU Clock Controller Binding +------------------------------------------------ +The A53 CPU Clock Controller is hardware, which provides a combined +mux and divider functionality for the CPU clocks. It can choose between +a fixed rate clock and the dedicated A53 PLL. + +Required properties : +- compatible : shall contain: + + "qcom,a53cc" + +- reg : shall contain base register location and length + of the APCS region +- #clock-cells : shall contain 1 + +Example: + + apcs: syscon@b011000 { + compatible = "qcom,a53cc", "syscon"; + reg = <0x0b011000 0x1000>; + #clock-cells = <1>; + }; diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig index a889f0b14b54..59dfcdc340e4 100644 --- a/drivers/clk/qcom/Kconfig +++ b/drivers/clk/qcom/Kconfig @@ -159,3 +159,11 @@ config QCOM_A53PLL support for CPU frequencies above 1GHz. Say Y if you want to support CPU frequency scaling on devices such as MSM8916. + +config QCOM_A53CC + bool "A53 Clock Controller" + depends on COMMON_CLK_QCOM && QCOM_A53PLL + help + Support for the A53 clock controller on some Qualcomm devices. + Say Y if you want to support CPU frequency scaling on devices + such as MSM8916. diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile index d3e142e577b0..980a5d729aa4 100644 --- a/drivers/clk/qcom/Makefile +++ b/drivers/clk/qcom/Makefile @@ -30,4 +30,5 @@ obj-$(CONFIG_MSM_LCC_8960) += lcc-msm8960.o obj-$(CONFIG_MSM_MMCC_8960) += mmcc-msm8960.o obj-$(CONFIG_MSM_MMCC_8974) += mmcc-msm8974.o obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o +obj-$(CONFIG_QCOM_A53CC) += a53cc.o obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o diff --git a/drivers/clk/qcom/a53cc.c b/drivers/clk/qcom/a53cc.c new file mode 100644 index 000000000000..4d20db9da407 --- /dev/null +++ b/drivers/clk/qcom/a53cc.c @@ -0,0 +1,155 @@ +/* + * Copyright (c) 2016, Linaro Limited + * Copyright (c) 2014, The Linux Foundation. All rights reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 and + * only version 2 as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/cpu.h> +#include <linux/kernel.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> + +#include "clk-regmap.h" +#include "clk-regmap-mux-div.h" + +enum { + P_GPLL0, + P_A53PLL, +}; + +static const struct parent_map gpll0_a53cc_map[] = { + { P_GPLL0, 4 }, + { P_A53PLL, 5 }, +}; + +static const char * const gpll0_a53cc[] = { + "gpll0_vote", + "a53pll", +}; + +static const struct regmap_config a53cc_regmap_config = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .max_register = 0x1000, + .fast_io = true, + .val_format_endian = REGMAP_ENDIAN_LITTLE, +}; + +static const struct of_device_id qcom_a53cc_match_table[] = { + { .compatible = "qcom,a53cc" }, + { } +}; + +/* + * We use the notifier function for switching to a temporary safe configuration + * (mux and divider), while the a53 pll is reconfigured. + */ +static int a53cc_notifier_cb(struct notifier_block *nb, unsigned long event, + void *data) +{ + int ret = 0; + struct clk_regmap_mux_div *md = container_of(nb, + struct clk_regmap_mux_div, + clk_nb); + + if (event == PRE_RATE_CHANGE) + ret = __mux_div_set_src_div(md, md->safe_src, md->safe_div); + + return notifier_from_errno(ret); +} + +static int qcom_a53cc_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct clk_regmap_mux_div *a53cc; + struct resource *res; + void __iomem *base; + struct clk *pclk; + struct regmap *regmap; + struct clk_init_data init; + int ret; + + a53cc = devm_kzalloc(dev, sizeof(*a53cc), GFP_KERNEL); + if (!a53cc) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + base = devm_ioremap_resource(dev, res); + if (IS_ERR(base)) + return PTR_ERR(base); + + a53cc->reg_offset = 0x50, + a53cc->hid_width = 5, + a53cc->hid_shift = 0, + a53cc->src_width = 3, + a53cc->src_shift = 8, + a53cc->safe_src = 4, + a53cc->safe_div = 3, + a53cc->parent_map = gpll0_a53cc_map, + + init.name = "a53mux", + init.parent_names = gpll0_a53cc, + init.num_parents = 2, + init.ops = &clk_regmap_mux_div_ops, + init.flags = CLK_SET_RATE_PARENT; + a53cc->clkr.hw.init = &init; + + pclk = __clk_lookup(gpll0_a53cc[1]); + if (!pclk) + return -EPROBE_DEFER; + + a53cc->clk_nb.notifier_call = a53cc_notifier_cb; + ret = clk_notifier_register(pclk, &a53cc->clk_nb); + if (ret) { + dev_err(dev, "failed to register clock notifier: %d\n", ret); + return ret; + } + + regmap = devm_regmap_init_mmio(dev, base, &a53cc_regmap_config); + if (IS_ERR(regmap)) { + ret = PTR_ERR(regmap); + dev_err(dev, "failed to init regmap mmio: %d\n", ret); + goto err; + } + + a53cc->clkr.regmap = regmap; + + ret = devm_clk_register_regmap(dev, &a53cc->clkr); + if (ret) { + dev_err(dev, "failed to register regmap clock: %d\n", ret); + goto err; + } + + ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_simple_get, + &a53cc->clkr.hw); + if (ret) { + dev_err(dev, "failed to add clock provider: %d\n", ret); + goto err; + } + + return 0; +err: + clk_notifier_unregister(pclk, &a53cc->clk_nb); + return ret; +} + +static struct platform_driver qcom_a53cc_driver = { + .probe = qcom_a53cc_probe, + .driver = { + .name = "qcom-a53cc", + .of_match_table = qcom_a53cc_match_table, + }, +}; + +builtin_platform_driver(qcom_a53cc_driver);
Add a driver for the A53 Clock Controller. It is a hardware block that implements a combined mux and half integer divider functionality. It can choose between a fixed-rate clock or the dedicated A53 PLL. The source and the divider can be set both at the same time. This is required for enabling CPU frequency scaling on platforms like MSM8916. Signed-off-by: Georgi Djakov <georgi.djakov@linaro.org> --- .../devicetree/bindings/clock/qcom,a53cc.txt | 22 +++ drivers/clk/qcom/Kconfig | 8 ++ drivers/clk/qcom/Makefile | 1 + drivers/clk/qcom/a53cc.c | 155 +++++++++++++++++++++ 4 files changed, 186 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/qcom,a53cc.txt create mode 100644 drivers/clk/qcom/a53cc.c -- To unsubscribe from this list: send the line "unsubscribe linux-clk" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html