| Message ID | 1501857104-11279-1-git-send-email-sudeep.holla@arm.com |
|---|---|
| Headers | show |
| Series | firmware: ARM System Control and Management Interface(SCMI) support | expand |
On Fri, Aug 4, 2017 at 8:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: .... > +int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer) > +{ > + int ret; > + int timeout; > + struct scmi_info *info = handle_to_scmi_info(handle); > + struct device *dev = info->dev; > + > + ret = mbox_send_message(info->tx_chan, xfer); > NAK This is still not fixed. For the reasons mentioned many times like here... https://lkml.org/lkml/2017/7/7/465 -- 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 Tue, Aug 8, 2017 at 2:59 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > On 08/08/17 03:46, Jassi Brar wrote: >> On Fri, Aug 4, 2017 at 8:01 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> >> .... >>> +int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer) >>> +{ >>> + int ret; >>> + int timeout; >>> + struct scmi_info *info = handle_to_scmi_info(handle); >>> + struct device *dev = info->dev; >>> + >>> + ret = mbox_send_message(info->tx_chan, xfer); >>> >> NAK >> >> This is still not fixed. For the reasons mentioned many times like >> here... https://lkml.org/lkml/2017/7/7/465 >> > > Since SCPI and SCMI are based on doorbell designs like ACPI PCC, they > can't send any data as all the data are part of shared memory. > > What data needs to be sent from SCPI/SCMI as part of mbox_send_message > in your opinion ? I can't think of any generic way to form this data. > https://lkml.org/lkml/2017/7/7/290 -- 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 04-08-17, 15:31, Sudeep Holla wrote: > The cpufreq core provides option for drivers to implement fast_switch > callback which is invoked for frequency switching from interrupt context. > > This patch adds support for fast_switch callback in SCMI cpufreq driver > by making use of polling based SCMI transfer. It also sets the flag > fast_switch_possible. > > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> > Cc: Viresh Kumar <viresh.kumar@linaro.org> > Cc: linux-pm@vger.kernel.org > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/cpufreq/scmi-cpufreq.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c > index 034359cafea5..cb1084cb1ef1 100644 > --- a/drivers/cpufreq/scmi-cpufreq.c > +++ b/drivers/cpufreq/scmi-cpufreq.c > @@ -61,6 +61,19 @@ scmi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) > return perf_ops->freq_set(priv->handle, priv->domain_id, freq, false); > } > > +static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy, > + unsigned int target_freq) > +{ > + struct scmi_data *priv = policy->driver_data; > + struct scmi_perf_ops *perf_ops = priv->handle->perf_ops; > + > + if (!perf_ops->freq_set(priv->handle, priv->domain_id, > + target_freq * 1000, true)) > + return target_freq; > + > + return CPUFREQ_ENTRY_INVALID; > +} This is very much similar to the target routine, perhaps we should write another local routine which is used by both target and fast switch. Do we guarantee that the frequency is changed by the time this routine returns? Or we just send a SCMI request and return back ? If we just send the request and don't wait for freq to get changed, what protects another scmi_cpufreq_fast_switch() to get called before the first one is finished? And what will happen on that call ? > static int > scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask) > { > @@ -164,6 +177,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) > > policy->cpuinfo.transition_latency = latency; > > + policy->fast_switch_possible = true; > return 0; > > out_free_cpufreq_table: > @@ -180,6 +194,7 @@ static int scmi_cpufreq_exit(struct cpufreq_policy *policy) > { > struct scmi_data *priv = policy->driver_data; > > + policy->fast_switch_possible = false; > cpufreq_cooling_unregister(priv->cdev); > dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); > dev_pm_opp_cpumask_remove_table(policy->related_cpus); > @@ -228,6 +243,7 @@ static struct cpufreq_driver scmi_cpufreq_driver = { > .init = scmi_cpufreq_init, > .exit = scmi_cpufreq_exit, > .ready = scmi_cpufreq_ready, > + .fast_switch = scmi_cpufreq_fast_switch, Maybe add it right after target_index ? -- viresh -- 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 09/08/17 05:18, Viresh Kumar wrote: > On 04-08-17, 15:31, Sudeep Holla wrote: > > I don't think its the Microsoft exchange server which screwed up tabs and > spaces, but you. > Indeed, copy paste to blame ;) >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm >> index 2011fec2d6ad..c34633855bc7 100644 >> --- a/drivers/cpufreq/Kconfig.arm >> +++ b/drivers/cpufreq/Kconfig.arm >> @@ -215,6 +215,17 @@ config ARM_SA1100_CPUFREQ >> config ARM_SA1110_CPUFREQ >> bool >> >> +config ARM_SCMI_CPUFREQ >> + tristate "SCMI based CPUfreq driver" > > You have used spaces here instead of tab and at multiple other places, can you > please fix them all ? > Done locally. >> + depends on ARM_SCMI_PROTOCOL || COMPILE_TEST >> + select PM_OPP >> + help >> + This adds the CPUfreq driver support for ARM platforms using SCMI >> + protocol for CPU power management. >> + >> + This driver uses SCMI Message Protocol driver to interact with the >> + firmware providing the CPU DVFS functionality. >> + >> config ARM_SCPI_CPUFREQ >> tristate "SCPI based CPUfreq driver" >> depends on ARM_BIG_LITTLE_CPUFREQ && ARM_SCPI_PROTOCOL && COMMON_CLK_SCPI >> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile >> index ab3a42cd29ef..4810b45568d3 100644 >> --- a/drivers/cpufreq/Makefile >> +++ b/drivers/cpufreq/Makefile >> @@ -72,6 +72,7 @@ obj-$(CONFIG_ARM_S3C64XX_CPUFREQ) += s3c64xx-cpufreq.o >> obj-$(CONFIG_ARM_S5PV210_CPUFREQ) += s5pv210-cpufreq.o >> obj-$(CONFIG_ARM_SA1100_CPUFREQ) += sa1100-cpufreq.o >> obj-$(CONFIG_ARM_SA1110_CPUFREQ) += sa1110-cpufreq.o >> +obj-$(CONFIG_ARM_SCMI_CPUFREQ) += scmi-cpufreq.o >> obj-$(CONFIG_ARM_SCPI_CPUFREQ) += scpi-cpufreq.o >> obj-$(CONFIG_ARM_SPEAR_CPUFREQ) += spear-cpufreq.o >> obj-$(CONFIG_ARM_STI_CPUFREQ) += sti-cpufreq.o >> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c >> new file mode 100644 >> index 000000000000..034359cafea5 >> --- /dev/null >> +++ b/drivers/cpufreq/scmi-cpufreq.c >> @@ -0,0 +1,268 @@ >> +/* >> + * System Control and Power Interface (SCMI) based CPUFreq Interface driver >> + * >> + * Copyright (C) 2017 ARM Ltd. >> + * Sudeep Holla <sudeep.holla@arm.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any >> + * kind, whether express or implied; without even the implied warranty >> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include <linux/cpu.h> >> +#include <linux/cpufreq.h> >> +#include <linux/cpumask.h> >> +#include <linux/cpu_cooling.h> >> +#include <linux/export.h> >> +#include <linux/module.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_opp.h> >> +#include <linux/slab.h> >> +#include <linux/scmi_protocol.h> >> +#include <linux/types.h> >> + >> +struct scmi_data { >> + int domain_id; >> + struct device *cpu_dev; >> + struct thermal_cooling_device *cdev; >> + const struct scmi_handle *handle; > > This stores the same handle pointer which is stored in the global variable > below. Right? Why keep a local variable here at all ? > Yes, you are right. Initially, started with just private pointers and then added global. I was thinking of calling devm_scmi_handle_get per policy to reflect the refcount correctly and drop global variable. Let me know what you think. >> +}; >> + >> +static const struct scmi_handle *handle; >> + >> +unsigned int scmi_cpufreq_get_rate(unsigned int cpu) >> +{ >> + int ret; >> + unsigned long rate; >> + struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu); >> + struct scmi_data *priv = policy->driver_data; >> + struct scmi_perf_ops *perf_ops = priv->handle->perf_ops; > > Normally people prefer to keep these definitions in decreasing order of their > lengths. i.e. ret and rate would be defined in the last line. Though I would > leave it to you to decide. > I too prefer that, will fix that. >> + >> + ret = perf_ops->freq_get(priv->handle, priv->domain_id, &rate, false); >> + if (ret) >> + return CPUFREQ_ENTRY_INVALID; > > This is something special which is used only when we are returning indexes and > I am not sure if this will have benefit here. I will rather return 0 here. > That's what other drivers are doing. > Indeed had 0 initially but changed as per Juri's suggestion. But is 0 treated as failure and still running at current OPP ? and not 0KHz I assume. >> + return rate / 1000; >> +} >> + >> +static int >> +scmi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) >> +{ >> + struct scmi_data *priv = policy->driver_data; >> + struct scmi_perf_ops *perf_ops = priv->handle->perf_ops; >> + u64 freq = policy->freq_table[index].frequency * 1000; >> + >> + return perf_ops->freq_set(priv->handle, priv->domain_id, freq, false); >> +} > > I suppose any CPU can change the frequency of any other CPU here, right? You > must set policy->dvfs_possible_from_any_cpu = true, from ->init() then. > OK, I missed to see something like that exists, will do. >> +static int >> +scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask) >> +{ >> + int cpu, domain, ret = 0; > > You don't need to initialize ret here and I would rather name it tdomain or > something else. ret is a lot used to store error/success values, which isn't > your case. > Agreed. >> + struct device *tcpu_dev; >> + >> + domain = handle->perf_ops->device_domain_id(cpu_dev); >> + if (domain < 0) >> + return domain; >> + >> + cpumask_set_cpu(cpu_dev->id, cpumask); > > The mask already have this set from the core, you don't need to do it again. > Cool, wasn't aware of that. >> + for_each_possible_cpu(cpu) { >> + if (cpu == cpu_dev->id) >> + continue; >> + >> + tcpu_dev = get_cpu_device(cpu); >> + if (!tcpu_dev) >> + continue; >> + >> + ret = handle->perf_ops->device_domain_id(tcpu_dev); >> + if (ret == domain) >> + cpumask_set_cpu(cpu, cpumask); >> + } >> + >> + return 0; >> +} >> + >> +static int scmi_cpufreq_init(struct cpufreq_policy *policy) >> +{ >> + int ret; >> + unsigned int latency; >> + struct device *cpu_dev; >> + struct scmi_data *priv; >> + struct cpufreq_frequency_table *freq_table; >> + >> + cpu_dev = get_cpu_device(policy->cpu); >> + if (!cpu_dev) { >> + pr_err("failed to get cpu%d device\n", policy->cpu); >> + return -ENODEV; >> + } >> + >> + ret = handle->perf_ops->add_opps_to_device(cpu_dev); >> + if (ret) { >> + dev_warn(cpu_dev, "failed to add opps to the device\n"); >> + return ret; >> + } >> + >> + ret = scmi_get_sharing_cpus(cpu_dev, policy->cpus); >> + if (ret) { >> + dev_warn(cpu_dev, "failed to get sharing cpumask\n"); >> + return ret; >> + } >> + >> + ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus); >> + if (ret) { >> + dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", >> + __func__, ret); >> + return ret; >> + } >> + >> + /* >> + * But we need OPP table to function so if it is not there let's >> + * give platform code chance to provide it for us. >> + */ > > How are we getting the OPPs? DT or non DT ? > Non DT :), from the firmware. >> + ret = dev_pm_opp_get_opp_count(cpu_dev); >> + if (ret <= 0) { >> + dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n"); >> + ret = -EPROBE_DEFER; >> + goto out_free_opp; >> + } >> + >> + priv = kzalloc(sizeof(*priv), GFP_KERNEL); >> + if (!priv) { >> + ret = -ENOMEM; >> + goto out_free_opp; >> + } >> + >> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table); >> + if (ret) { >> + dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret); >> + goto out_free_priv; >> + } >> + >> + priv->handle = handle; >> + priv->cpu_dev = cpu_dev; >> + priv->domain_id = handle->perf_ops->device_domain_id(cpu_dev); >> + >> + policy->driver_data = priv; >> + >> + ret = cpufreq_table_validate_and_show(policy, freq_table); >> + if (ret) { >> + dev_err(cpu_dev, "%s: invalid frequency table: %d\n", __func__, >> + ret); >> + goto out_free_cpufreq_table; >> + } >> + >> + latency = handle->perf_ops->get_transition_latency(cpu_dev); >> + if (!latency) >> + latency = CPUFREQ_ETERNAL; >> + >> + policy->cpuinfo.transition_latency = latency; >> + >> + return 0; >> + >> +out_free_cpufreq_table: >> + dev_pm_opp_free_cpufreq_table(cpu_dev, &freq_table); >> +out_free_priv: >> + kfree(priv); >> +out_free_opp: >> + dev_pm_opp_cpumask_remove_table(policy->cpus); >> + >> + return ret; >> +} >> + >> +static int scmi_cpufreq_exit(struct cpufreq_policy *policy) >> +{ >> + struct scmi_data *priv = policy->driver_data; >> + >> + cpufreq_cooling_unregister(priv->cdev); >> + dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); >> + dev_pm_opp_cpumask_remove_table(policy->related_cpus); >> + kfree(priv); > > I would rather swap the above two lines to keep the same order as in probe. > Though nothing would fail with the current code as well. > Sure. >> + >> + return 0; >> +} >> + >> +static void scmi_cpufreq_ready(struct cpufreq_policy *policy) >> +{ >> + struct scmi_data *priv = policy->driver_data; >> + struct device_node *np = of_node_get(priv->cpu_dev->of_node); >> + >> + if (WARN_ON(!np)) >> + return; >> + >> + if (of_find_property(np, "#cooling-cells", NULL)) { >> + u32 pcoeff = 0; >> + >> + of_property_read_u32(np, "dynamic-power-coefficient", >> + &pcoeff); >> + >> + priv->cdev = of_cpufreq_power_cooling_register(np, policy, >> + pcoeff, NULL); >> + if (IS_ERR(priv->cdev)) { >> + dev_err(priv->cpu_dev, >> + "running cpufreq without cooling device: %ld\n", >> + PTR_ERR(priv->cdev)); >> + >> + priv->cdev = NULL; >> + } >> + } >> + >> + of_node_put(np); >> +} >> + >> +static struct cpufreq_driver scmi_cpufreq_driver = { >> + .name = "scmi", >> + .flags = CPUFREQ_STICKY | >> + CPUFREQ_HAVE_GOVERNOR_PER_POLICY | >> + CPUFREQ_NEED_INITIAL_FREQ_CHECK, >> + .verify = cpufreq_generic_frequency_table_verify, >> + .attr = cpufreq_generic_attr, >> + .target_index = scmi_cpufreq_set_target, >> + .get = scmi_cpufreq_get_rate, >> + .init = scmi_cpufreq_init, >> + .exit = scmi_cpufreq_exit, >> + .ready = scmi_cpufreq_ready, >> +}; > > Above block has lots of space/tab issues. Can you please use tabs before "=" > instead? > OK, again copy pasted from some other driver ;) >> +static int scmi_cpufreq_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + >> + handle = devm_scmi_handle_get(&pdev->dev); > > What code is creating this pdev ? > SCMI driver, once it finds the performance protocol is available and setup/initialized. Thanks for the review. -- Regards, Sudeep -- 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 09/08/17 05:28, Viresh Kumar wrote: > On 04-08-17, 15:31, Sudeep Holla wrote: >> The cpufreq core provides option for drivers to implement fast_switch >> callback which is invoked for frequency switching from interrupt context. >> >> This patch adds support for fast_switch callback in SCMI cpufreq driver >> by making use of polling based SCMI transfer. It also sets the flag >> fast_switch_possible. >> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net> >> Cc: Viresh Kumar <viresh.kumar@linaro.org> >> Cc: linux-pm@vger.kernel.org >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/cpufreq/scmi-cpufreq.c | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/cpufreq/scmi-cpufreq.c b/drivers/cpufreq/scmi-cpufreq.c >> index 034359cafea5..cb1084cb1ef1 100644 >> --- a/drivers/cpufreq/scmi-cpufreq.c >> +++ b/drivers/cpufreq/scmi-cpufreq.c >> @@ -61,6 +61,19 @@ scmi_cpufreq_set_target(struct cpufreq_policy *policy, unsigned int index) >> return perf_ops->freq_set(priv->handle, priv->domain_id, freq, false); >> } >> >> +static unsigned int scmi_cpufreq_fast_switch(struct cpufreq_policy *policy, >> + unsigned int target_freq) >> +{ >> + struct scmi_data *priv = policy->driver_data; >> + struct scmi_perf_ops *perf_ops = priv->handle->perf_ops; >> + >> + if (!perf_ops->freq_set(priv->handle, priv->domain_id, >> + target_freq * 1000, true)) >> + return target_freq; >> + >> + return CPUFREQ_ENTRY_INVALID; >> +} > > This is very much similar to the target routine, perhaps we should write another > local routine which is used by both target and fast switch. > Just one difference, fast switch uses polling based mailbox while target_index uses interrupt based. I thought initially to reuse, but it's comes done to just perf_ops->freq_set, so dropped that idea. > Do we guarantee that the frequency is changed by the time this routine returns? No, firmware may return acknowledging the request not it's completion. > Or we just send a SCMI request and return back ? > Yes, exactly. > If we just send the request and don't wait for freq to get changed, what > protects another scmi_cpufreq_fast_switch() to get called before the first one > is finished? And what will happen on that call ? Firmware needs to serialize or override based on the timing of the two consecutive requests. > >> static int >> scmi_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask) >> { >> @@ -164,6 +177,7 @@ static int scmi_cpufreq_init(struct cpufreq_policy *policy) >> >> policy->cpuinfo.transition_latency = latency; >> >> + policy->fast_switch_possible = true; >> return 0; >> >> out_free_cpufreq_table: >> @@ -180,6 +194,7 @@ static int scmi_cpufreq_exit(struct cpufreq_policy *policy) >> { >> struct scmi_data *priv = policy->driver_data; >> >> + policy->fast_switch_possible = false; >> cpufreq_cooling_unregister(priv->cdev); >> dev_pm_opp_free_cpufreq_table(priv->cpu_dev, &policy->freq_table); >> dev_pm_opp_cpumask_remove_table(policy->related_cpus); >> @@ -228,6 +243,7 @@ static struct cpufreq_driver scmi_cpufreq_driver = { >> .init = scmi_cpufreq_init, >> .exit = scmi_cpufreq_exit, >> .ready = scmi_cpufreq_ready, >> + .fast_switch = scmi_cpufreq_fast_switch, > > Maybe add it right after target_index ? > Done -- Regards, Sudeep -- 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 09/08/17 11:06, Viresh Kumar wrote: > On 09-08-17, 10:59, Sudeep Holla wrote: >> On 09/08/17 05:18, Viresh Kumar wrote: > >>> This stores the same handle pointer which is stored in the global variable >>> below. Right? Why keep a local variable here at all ? >> >> Yes, you are right. Initially, started with just private pointers and >> then added global. I was thinking of calling devm_scmi_handle_get per >> policy to reflect the refcount correctly and drop global variable. Let >> me know what you think. > > A refcount of 1 should be fine as well, i.e. For the cpufreq driver. Why would > SCMI care if we manage multiple policies here ? Unless it makes something within > SCMI core better. > Not really, just we can get rid of global pointer which may be need in system with multiple scmi instances, but that's long way to go. >>> This is something special which is used only when we are returning indexes and >>> I am not sure if this will have benefit here. I will rather return 0 here. >>> That's what other drivers are doing. >> >> Indeed had 0 initially but changed as per Juri's suggestion. > > Maybe he suggested doing that in the fast switch routine ? As that's the normal > protocol there. Though I have sent a patch today to propose using 0 there as > well (you cc'd). > Yes, saw that. I have changed both to 0 for now. I will watch that thread and update if necessary before next posting. >> But is 0 >> treated as failure and still running at current OPP ? > > You have used that in the ->get() routine. So the OPP isn't changing, but we are > just trying to fetch it. cpufreq core doesn't do a lot with the value returned > from here, but at one place we break early if 0 is returned. And so all drivers > are returning that. > Agreed, I assumed _INVALID is new thing and changed at both target_indes and fast_switch. >> and not 0KHz I assume. > > Yeah, 0 KHz is dead CPU really :) > :) >>> I suppose any CPU can change the frequency of any other CPU here, right? You >>> must set policy->dvfs_possible_from_any_cpu = true, from ->init() then. >>> >> >> OK, I missed to see something like that exists, will do. > > Fairly recent stuff, present in pm/linux-next only. > Oh OK. >>>> + /* >>>> + * But we need OPP table to function so if it is not there let's >>>> + * give platform code chance to provide it for us. >>>> + */ >>> >>> How are we getting the OPPs? DT or non DT ? >>> >> >> Non DT :), from the firmware. > > I would improve the above comment in that case to clearly say that OPPs are > added by the platform, lets wait for it. > Done -- Regards, Sudeep -- 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 09/08/17 11:13, Viresh Kumar wrote: > On 09-08-17, 11:09, Sudeep Holla wrote: >> Firmware needs to serialize or override based on the timing of the two >> consecutive requests. > > Maybe add a comment over that routine and detail its working a bit? Like its not > synchronous, etc. I expect that you would also add a callback with SCMI, so that > the cpufreq driver is notified when frequency is changed ? > OK, will do. Yes, I don't have firmware implementing notifications yet, so waiting until I can test it. -- Regards, Sudeep -- 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 Fri, Aug 04, 2017 at 03:31:28PM +0100, Sudeep Holla wrote: > This patch adds devicetree binding for System Control and Management > Interface (SCMI) Message Protocol used between the Application Cores(AP) > and the System Control Processor(SCP). The MHU peripheral provides a > mechanism for inter-processor communication between SCP's M3 processor > and AP. > > SCP offers control and management of the core/cluster power states, > various power domain DVFS including the core/cluster, certain system > clocks configuration, thermal sensors and many others. > > SCMI protocol is developed as better replacement to the existing SCPI > which is not flexible and easily extensible. > > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > Documentation/devicetree/bindings/arm/arm,scmi.txt | 174 +++++++++++++++++++++ > 1 file changed, 174 insertions(+) > create mode 100644 Documentation/devicetree/bindings/arm/arm,scmi.txt > > diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt b/Documentation/devicetree/bindings/arm/arm,scmi.txt > new file mode 100644 > index 000000000000..33c16be58e72 > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt > @@ -0,0 +1,174 @@ > +System Control and Management Interface (SCMI) Message Protocol > +---------------------------------------------------------- > + > +The SCMI is intended to allow agents such as OSPM to manage various functions > +that are provided by the hardware platform it is running on, including power > +and performance functions. > + > +This binding is intended to define the interface the firmware implementing > +the SCMI as described in ARM document number ARM DUI 0922B ("ARM System Control > +and Management Interface Platform Design Document")[0] provide for OSPM in > +the device tree. > + > +Required properties: Please define this is a subnode of /firmware node. > + > +- compatible : shall be "arm,scmi" > +- mboxes: List of phandle and mailbox channel specifiers. It should contain > + exactly one or two mailboxes, one for transmitting messages("tx") > + and another optional for receiving the notifications("rx") if > + supported. > +- mbox-names: shall be "tx" or "rx" > +- shmem : List of phandle pointing to the shared memory(SHM) area as per > + generic mailbox client binding. > + > +See Documentation/devicetree/bindings/mailbox/mailbox.txt for more details > +about the generic mailbox controller and client driver bindings. > + > +The mailbox is the only permitted method of calling the SCMI firmware. > +Mailbox doorbell is used as a mechanism to alert the presence of a > +messages and/or notification. > + > +Each protocol supported shall have a sub-node with corresponding compatible > +as described in the following sections. If the platform supports dedicated > +communication channel for a particular protocol, the 3 properties namely: > +mboxes, mbox-names and shmem shall be present in the sub-node corresponding > +to that protocol. > + > +Clock/Performance bindings for the clocks/OPPs based on SCMI Message Protocol > +------------------------------------------------------------ > + > +This binding uses the common clock binding[1]. > + > +Required properties: > +- #clock-cells : Should be 1. Contains the Clock ID value used by SCMI commands. > + > +Power domain bindings for the power domains based on SCMI Message Protocol > +------------------------------------------------------------ > + > +This binding uses the generic power domain binding[4]. > + > +PM domain providers > +=================== > + > +Required properties: > + - #power-domain-cells : Should be 1. Contains the device or the power > + domain ID value used by SCMI commands. > + > +PM domain consumers > +=================== How consumers work is already defined elsewhere. > + > +Required properties: > + - power-domains : A phandle and PM domain specifier as defined by bindings of > + the power controller specified by phandle. > + > +Sensor bindings for the sensors based on SCMI Message Protocol > +-------------------------------------------------------------- > +SCMI provides an API to access the various sensors on the SoC. > + > +Required properties: > +- #thermal-sensor-cells: should be set to 1. This property follows the > + thermal device tree bindings[2]. > + > + Valid cell values are raw identifiers (Sensor ID) > + as used by the firmware. Refer to platform details > + for your implementation for the IDs to use. > + > +SRAM and Shared Memory for SCMI > +------------------------------- > + > +A small area of SRAM is reserved for SCMI communication between application > +processors and SCP. > + > +The properties should follow the generic mmio-sram description found in [3] > + > +Each sub-node represents the reserved area for SCMI. > + > +Required sub-node properties: > +- reg : The base offset and size of the reserved area with the SRAM > +- compatible : should be "arm,scmi-shmem" for Non-secure SRAM based > + shared memory > + > +[0] http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/index.html > +[1] Documentation/devicetree/bindings/clock/clock-bindings.txt > +[2] Documentation/devicetree/bindings/thermal/thermal.txt > +[3] Documentation/devicetree/bindings/sram/sram.txt > +[4] Documentation/devicetree/bindings/power/power_domain.txt > + > +Example: > + > +sram: sram@50000000 { > + compatible = "mmio-sram"; > + reg = <0x0 0x50000000 0x0 0x10000>; > + > + #address-cells = <1>; > + #size-cells = <1>; > + ranges = <0 0x0 0x50000000 0x10000>; > + > + cpu_scp_lpri: scp-shmem@0 { > + compatible = "arm,scmi-shmem"; > + reg = <0x0 0x200>; > + }; > + > + cpu_scp_hpri: scp-shmem@200 { > + compatible = "arm,scmi-shmem"; > + reg = <0x200 0x200>; > + }; > +}; > + > +mailbox: mailbox0@40000000 { > + .... > + #mbox-cells = <1>; > +}; > + > +scmi_protocol: scmi@2e000000 { The unit address is not valid. > + compatible = "arm,scmi"; > + method = "mailbox-doorbell"; Is this not implied by the mboxes property? > + mboxes = <&mailbox 0 &mailbox 1>; > + shmem = <&cpu_scp_lpri &cpu_scp_hpri>; > + #address-cells = <1>; > + #size-cells = <0>; > + > + scmi_devpd: protocol@11 { > + reg = <0x11>; > + #power-domain-cells = <1>; > + }; > + > + scmi_dvfs: protocol@13 { > + reg = <0x13>; > + #clock-cells = <1>; > + }; > + > + scmi_clk: protocol@14 { > + reg = <0x14>; > + #clock-cells = <1>; > + }; > + > + scmi_sensors0: protocol@15 { > + reg = <0x15>; > + #thermal-sensor-cells = <1>; > + }; > +}; > + > +cpu@0 { > + ... > + reg = <0 0>; > + clocks = <&scmi_dvfs 0>; > +}; > + > +hdlcd@7ff60000 { > + ... > + reg = <0 0x7ff60000 0 0x1000>; > + clocks = <&scmi_clk 4>; > + power-domains = <&scmi_devpd 1>; > +}; > + > +thermal-zones { > + soc_thermal { > + polling-delay-passive = <100>; > + polling-delay = <1000>; > + > + /* sensor ID */ > + thermal-sensors = <&scmi_sensors0 3>; > + ... > + }; > +}; > -- > 2.7.4 > -- 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 Guenter, On 07/08/17 13:25, Sudeep Holla wrote: > > > On 04/08/17 20:32, Guenter Roeck wrote: >> On Fri, Aug 04, 2017 at 03:31:42PM +0100, Sudeep Holla wrote: > > [...] > >>> + platform_set_drvdata(pdev, scmi_sensors); >>> + >>> + hwdev = devm_hwmon_device_register_with_groups(dev, "scmi_sensors", >>> + scmi_sensors, >>> + scmi_sensors->groups); >> >> Can you rework this to use devm_hwmon_device_register_with_info(), >> and if possible let it handle the thermal registration ? >> > > Thanks for the pointers. I will check on the possibility and use it if > possible. > I had a look at devm_hwmon_device_register_with_info. It mostly deals with constant structures where all the attributes are known at the boot time. In case of SCMI, the firmware presents all the sensors and all the information(type, scale, name/label, ...etc). Unless I create those structures dynamically and typecast as const during probe, I can't get it working. Are you OK with that ? If so, I can try making those changes. -- Regards, Sudeep -- 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 08/04, Sudeep Holla wrote: > diff --git a/drivers/clk/clk-scmi.c b/drivers/clk/clk-scmi.c > new file mode 100644 > index 000000000000..37f98a6439a0 > --- /dev/null > +++ b/drivers/clk/clk-scmi.c > @@ -0,0 +1,216 @@ > +/* > + * System Control and Power Interface (SCMI) Protocol based clock driver > + * > + * Copyright (C) 2017 ARM Ltd. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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. > + * > + * You should have received a copy of the GNU General Public License along with > + * this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/clk-provider.h> > +#include <linux/device.h> > +#include <linux/err.h> > +#include <linux/of.h> > +#include <linux/module.h> > +#include <linux/of_platform.h> Is this include used? > +#include <linux/platform_device.h> > +#include <linux/scmi_protocol.h> > + > +struct scmi_clk { > + u32 id; > + struct clk_hw hw; > + const struct scmi_clock_info *info; > + const struct scmi_handle *handle; > +}; > + > +#define to_scmi_clk(clk) container_of(clk, struct scmi_clk, hw) > + > +static unsigned long scmi_clk_recalc_rate(struct clk_hw *hw, > + unsigned long parent_rate) > +{ > + int ret; > + u64 rate; > + struct scmi_clk *clk = to_scmi_clk(hw); > + > + ret = clk->handle->clk_ops->rate_get(clk->handle, clk->id, &rate); > + if (ret) > + return 0; > + return rate; > +} > + > +static long scmi_clk_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + u64 fmin, fmax, ftmp; > + struct scmi_clk *clk = to_scmi_clk(hw); > + > + /* > + * We can't figure out what rate it will be, so just return the > + * rate back to the caller. scmi_clk_recalc_rate() will be called > + * after the rate is set and we'll know what rate the clock is > + * running at then. > + */ > + if (clk->info->rate_discrete) > + return rate; > + > + fmin = clk->info->range.min_rate; > + fmax = clk->info->range.max_rate; > + for (ftmp = fmin; ftmp <= fmax; ftmp += clk->info->range.step_size) { > + if (ftmp >= rate) { > + if (ftmp <= fmax) > + fmax = ftmp; > + break; > + } else if (ftmp >= fmin) { > + fmin = ftmp; > + } > + } Are the max_rate and min_rate potentially unaligned with the step_size? Do any of these things change at runtime? It seems like we could do some simple math instead of this for loop unless I missed something. > + return fmax != clk->info->range.max_rate ? fmax : fmin; > +} > + > +static int scmi_clk_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + > + return clk->handle->clk_ops->rate_set(clk->handle, clk->id, 0, rate); > +} > + > +static int scmi_clk_enable(struct clk_hw *hw) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + > + return clk->handle->clk_ops->enable(clk->handle, clk->id); > +} > + > +static void scmi_clk_disable(struct clk_hw *hw) > +{ > + struct scmi_clk *clk = to_scmi_clk(hw); > + > + clk->handle->clk_ops->disable(clk->handle, clk->id); > +} > + > +static const struct clk_ops scmi_clk_ops = { > + .recalc_rate = scmi_clk_recalc_rate, > + .round_rate = scmi_clk_round_rate, > + .set_rate = scmi_clk_set_rate, > + /* > + * We can't provide enable/disable callback as we can't perform the same > + * in atomic context. Since the clock framework provides standard API > + * clk_prepare_enable that helps cases using clk_enable in non-atomic > + * context, it should be fine providing prepare/unprepare. > + */ > + .prepare = scmi_clk_enable, > + .unprepare = scmi_clk_disable, > +}; > + > +static int scmi_clk_ops_init(struct device *dev, struct scmi_clk *sclk) > +{ > + int ret; > + struct clk_init_data init; Best to do = { } here in case we add something else to the init structure in future. > + > + init.flags = CLK_GET_RATE_NOCACHE; > + init.num_parents = 0; > + init.ops = &scmi_clk_ops; > + init.name = sclk->info->name; > + sclk->hw.init = &init; > + > + ret = devm_clk_hw_register(dev, &sclk->hw); > + if (!ret) > + clk_hw_set_rate_range(&sclk->hw, sclk->info->range.min_rate, > + sclk->info->range.max_rate); > + return ret; > +} > + > +static int scmi_clk_add(struct device *dev, struct device_node *np, > + const struct scmi_handle *handle) > +{ > + int idx, count, err; > + struct clk_hw **hws; > + struct clk_hw_onecell_data *clk_data; > + > + count = handle->clk_ops->count_get(handle); > + if (count < 0) { > + dev_err(dev, "%s: invalid clock output count\n", np->name); > + return -EINVAL; > + } > + > + clk_data = devm_kzalloc(dev, sizeof(*clk_data) + > + sizeof(*clk_data->hws) * count, GFP_KERNEL); > + if (!clk_data) > + return -ENOMEM; > + > + clk_data->num = count; > + hws = clk_data->hws; > + > + for (idx = 0; idx < count; idx++) { > + struct scmi_clk *sclk; > + > + sclk = devm_kzalloc(dev, sizeof(*sclk), GFP_KERNEL); > + if (!sclk) > + return -ENOMEM; > + > + sclk->info = handle->clk_ops->info_get(handle, idx); > + if (!sclk->info) { > + dev_dbg(dev, "invalid clock info for idx %d\n", idx); > + continue; > + } > + > + sclk->id = idx; > + sclk->handle = handle; > + > + err = scmi_clk_ops_init(dev, sclk); > + if (err) { > + dev_err(dev, "failed to register clock %d\n", idx); > + devm_kfree(dev, sclk); > + hws[idx] = NULL; > + } else { > + dev_dbg(dev, "Registered clock:%s\n", sclk->info->name); > + hws[idx] = &sclk->hw; > + } > + } > + > + return of_clk_add_hw_provider(np, of_clk_hw_onecell_get, clk_data); > +} > + > +static int scmi_clocks_remove(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + > + of_clk_del_provider(np); > + return 0; > +} > + > +static int scmi_clocks_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + const struct scmi_handle *handle = devm_scmi_handle_get(dev); > + > + if (IS_ERR_OR_NULL(handle) || !handle->clk_ops) > + return -EPROBE_DEFER; > + > + return scmi_clk_add(dev, np, handle); Why the function? We support more than just platform devices? Just fold scmi_clk_add() into this function instead please. > +} > + > +static struct platform_driver scmi_clocks_driver = { > + .driver = { > + .name = "scmi-clocks", > + }, > + .probe = scmi_clocks_probe, > + .remove = scmi_clocks_remove, > +}; > +module_platform_driver(scmi_clocks_driver); > + > +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>"); > +MODULE_DESCRIPTION("ARM SCMI clock driver"); > +MODULE_LICENSE("GPL v2"); > -- > 2.7.4 > -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project -- 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 Sudeep, I am not sure what the patch does is correct when having a big endian kernel dealing with scmi_shared_mem. Unless there is a reason not to have SCMI with big endian kernel, please see remarks below. On 04/08/17 15:31, Sudeep Holla wrote: > The SCMI is intended to allow OSPM to manage various functions that are > provided by the hardware platform it is running on, including power and > performance functions. SCMI provides two levels of abstraction, protocols > and transports. Protocols define individual groups of system control and > management messages. A protocol specification describes the messages > that it supports. Transports describe the method by which protocol > messages are communicated between agents and the platform. > > This patch adds basic infrastructure to manage the message allocation, > initialisation, packing/unpacking and shared memory management. > > Cc: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/Kconfig | 21 + > drivers/firmware/Makefile | 1 + > drivers/firmware/arm_scmi/Makefile | 2 + > drivers/firmware/arm_scmi/common.h | 74 ++++ > drivers/firmware/arm_scmi/driver.c | 774 +++++++++++++++++++++++++++++++++++++ > include/linux/scmi_protocol.h | 48 +++ > 6 files changed, 920 insertions(+) > create mode 100644 drivers/firmware/arm_scmi/Makefile > create mode 100644 drivers/firmware/arm_scmi/common.h > create mode 100644 drivers/firmware/arm_scmi/driver.c > create mode 100644 include/linux/scmi_protocol.h > > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig > index 6e4ed5a9c6fd..c3d1a12763ce 100644 > --- a/drivers/firmware/Kconfig > +++ b/drivers/firmware/Kconfig > @@ -19,6 +19,27 @@ config ARM_PSCI_CHECKER > on and off through hotplug, so for now torture tests and PSCI checker > are mutually exclusive. > > +config ARM_SCMI_PROTOCOL > + tristate "ARM System Control and Management Interface (SCMI) Message Protocol" > + depends on ARM || ARM64 || COMPILE_TEST > + depends on MAILBOX > + help > + ARM System Control and Management Interface (SCMI) protocol is a > + set of operating system-independent software interfaces that are > + used in system management. SCMI is extensible and currently provides > + interfaces for: Discovery and self-description of the interfaces > + it supports, Power domain management which is the ability to place > + a given device or domain into the various power-saving states that > + it supports, Performance management which is the ability to control > + the performance of a domain that is composed of compute engines > + such as application processors and other accelerators, Clock > + management which is the ability to set and inquire rates on platform > + managed clocks and Sensor management which is the ability to read > + sensor data, and be notified of sensor value. > + > + This protocol library provides interface for all the client drivers > + making use of the features offered by the SCMI. > + > config ARM_SCPI_PROTOCOL > tristate "ARM System Control and Power Interface (SCPI) Message Protocol" > depends on ARM || ARM64 || COMPILE_TEST > diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile > index a37f12e8d137..91d3ff62c653 100644 > --- a/drivers/firmware/Makefile > +++ b/drivers/firmware/Makefile > @@ -23,6 +23,7 @@ obj-$(CONFIG_QCOM_SCM_32) += qcom_scm-32.o > CFLAGS_qcom_scm-32.o :=$(call as-instr,.arch armv7-a\n.arch_extension sec,-DREQUIRES_SEC=1) -march=armv7-a > obj-$(CONFIG_TI_SCI_PROTOCOL) += ti_sci.o > > +obj-$(CONFIG_ARM_SCMI_PROTOCOL) += arm_scmi/ > obj-y += broadcom/ > obj-y += meson/ > obj-$(CONFIG_GOOGLE_FIRMWARE) += google/ > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile > new file mode 100644 > index 000000000000..58e94c95e523 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/Makefile > @@ -0,0 +1,2 @@ > +obj-$(CONFIG_ARM_SCMI_PROTOCOL) = arm_scmi.o > +arm_scmi-y = driver.o > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > new file mode 100644 > index 000000000000..a6829afc17e3 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/common.h > @@ -0,0 +1,74 @@ > +/* > + * System Control and Management Interface (SCMI) Message Protocol > + * driver common header file containing some definitions, structures > + * and function prototypes used in all the different SCMI protocols. > + * > + * Copyright (C) 2017 ARM Ltd. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/completion.h> > +#include <linux/scmi_protocol.h> > +#include <linux/types.h> > + > +/** > + * struct scmi_msg_hdr - Message(Tx/Rx) header > + * > + * @id: The identifier of the command being sent > + * @protocol_id: The identifier of the protocol used to send @id command > + * @seq: The token to identify the message. when a message/command returns, > + * the platform returns the whole message header unmodified including > + * the token. > + */ > +struct scmi_msg_hdr { > + u8 id; > + u8 protocol_id; > + u16 seq; > + u32 status; > + bool poll_completion; > +}; > + > +/** > + * struct scmi_msg - Message(Tx/Rx) structure > + * > + * @buf: Buffer pointer > + * @len: Length of data in the Buffer > + */ > +struct scmi_msg { > + void *buf; > + size_t len; > +}; > + > +/** > + * struct scmi_xfer - Structure representing a message flow > + * > + * @hdr: Transmit message header > + * @tx: Transmit message > + * @rx: Receive message, the buffer should be pre-allocated to store > + * message. If request-ACK protocol is used, we can reuse the same > + * buffer for the rx path as we use for the tx path. > + * @done: completion event > + */ > + > +struct scmi_xfer { > + struct scmi_msg_hdr hdr; > + struct scmi_msg tx; > + struct scmi_msg rx; > + struct completion done; > +}; > + > +void scmi_one_xfer_put(const struct scmi_handle *h, struct scmi_xfer *xfer); > +int scmi_do_xfer(const struct scmi_handle *h, struct scmi_xfer *xfer); > +int scmi_one_xfer_init(const struct scmi_handle *h, u8 msg_id, u8 prot_id, > + size_t tx_size, size_t rx_size, struct scmi_xfer **p); > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > new file mode 100644 > index 000000000000..139d6980f270 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -0,0 +1,774 @@ > +/* > + * System Control and Management Interface (SCMI) Message Protocol driver > + * > + * SCMI Message Protocol is used between the System Control Processor(SCP) > + * and the Application Processors(AP). The Message Handling Unit(MHU) > + * provides a mechanism for inter-processor communication between SCP's > + * Cortex M3 and AP. > + * > + * SCP offers control and management of the core/cluster power states, > + * various power domain DVFS including the core/cluster, certain system > + * clocks configuration, thermal sensors and many others. > + * > + * Copyright (C) 2017 ARM Ltd. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/bitmap.h> > +#include <linux/export.h> > +#include <linux/io.h> > +#include <linux/kernel.h> > +#include <linux/mailbox_client.h> > +#include <linux/module.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/semaphore.h> > +#include <linux/slab.h> > + > +#include "common.h" > + > +#define MSG_ID_SHIFT 0 > +#define MSG_ID_MASK 0xff > +#define MSG_TYPE_SHIFT 8 > +#define MSG_TYPE_MASK 0x3 > +#define MSG_PROTOCOL_ID_SHIFT 10 > +#define MSG_PROTOCOL_ID_MASK 0xff > +#define MSG_TOKEN_ID_SHIFT 18 > +#define MSG_TOKEN_ID_MASK 0x3ff > +#define MSG_XTRACT_TOKEN(header) \ > + (((header) >> MSG_TOKEN_ID_SHIFT) & MSG_TOKEN_ID_MASK) > + > +enum scmi_error_codes { > + SCMI_SUCCESS = 0, /* Success */ > + SCMI_ERR_SUPPORT = -1, /* Not supported */ > + SCMI_ERR_PARAMS = -2, /* Invalid Parameters */ > + SCMI_ERR_ACCESS = -3, /* Invalid access/permission denied */ > + SCMI_ERR_ENTRY = -4, /* Not found */ > + SCMI_ERR_RANGE = -5, /* Value out of range */ > + SCMI_ERR_BUSY = -6, /* Device busy */ > + SCMI_ERR_COMMS = -7, /* Communication Error */ > + SCMI_ERR_GENERIC = -8, /* Generic Error */ > + SCMI_ERR_HARDWARE = -9, /* Hardware Error */ > + SCMI_ERR_PROTOCOL = -10,/* Protocol Error */ > + SCMI_ERR_MAX > +}; > + > +/* List of all SCMI devices active in system */ > +static LIST_HEAD(scmi_list); > +/* Protection for the entire list */ > +static DEFINE_MUTEX(scmi_list_mutex); > + > +/** > + * struct scmi_xfers_info - Structure to manage transfer information > + * > + * @sem_xfer_count: Counting Semaphore for managing max simultaneous > + * Messages. > + * @xfer_block: Preallocated Message array > + * @xfer_alloc_table: Bitmap table for allocated messages. > + * Index of this bitmap table is also used for message > + * sequence identifier. > + * @xfer_lock: Protection for message allocation > + */ > +struct scmi_xfers_info { > + struct semaphore sem_xfer_count; > + struct scmi_xfer *xfer_block; > + unsigned long *xfer_alloc_table; > + /* protect transfer allocation */ > + spinlock_t xfer_lock; > +}; > + > +/** > + * struct scmi_desc - Description of SoC integration > + * > + * @max_rx_timeout_ms: Timeout for communication with SoC (in Milliseconds) > + * @max_msg: Maximum number of messages that can be pending > + * simultaneously in the system > + * @max_msg_size: Maximum size of data per message that can be handled. > + */ > +struct scmi_desc { > + int max_rx_timeout_ms; > + int max_msg; > + int max_msg_size; > +}; > + > +/** > + * struct scmi_info - Structure representing a SCMI instance > + * > + * @dev: Device pointer > + * @desc: SoC description for this instance > + * @handle: Instance of SCMI handle to send to clients > + * @cl: Mailbox Client > + * @tx_chan: Transmit mailbox channel > + * @rx_chan: Receive mailbox channel > + * @tx_payload: Transmit mailbox channel payload area > + * @rx_payload: Receive mailbox channel payload area > + * @minfo: Message info > + * @node: list head > + * @users: Number of users of this instance > + */ > +struct scmi_info { > + struct device *dev; > + const struct scmi_desc *desc; > + struct scmi_handle handle; > + struct mbox_client cl; > + struct mbox_chan *tx_chan; > + struct mbox_chan *rx_chan; > + void __iomem *tx_payload; > + void __iomem *rx_payload; > + struct scmi_xfers_info minfo; > + struct list_head node; > + int users; > +}; > + > +#define client_to_scmi_info(c) container_of(c, struct scmi_info, cl) > +#define handle_to_scmi_info(h) container_of(h, struct scmi_info, handle) > + > +/* > + * The SCP firmware providing SCM interface to OSPM and other agents must > + * execute only in little-endian mode as per SCMI specification, so any buffers > + * shared through SCMI should have their contents converted to little-endian > + */ > +struct scmi_shared_mem { > + __le32 reserved; > + __le32 channel_status; > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_ERROR BIT(1) > +#define SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE BIT(0) > + __le32 reserved1[2]; > + __le32 flags; > +#define SCMI_SHMEM_FLAG_INTR_ENABLED BIT(0) > + __le32 length; > + __le32 msg_header; > + u8 msg_payload[0]; > +}; > + > +static int scmi_linux_errmap[] = { > + /* better than switch case as long as return value is continuous */ > + 0, /* SCMI_SUCCESS */ > + -EOPNOTSUPP, /* SCMI_ERR_SUPPORT */ > + -EINVAL, /* SCMI_ERR_PARAM */ > + -EACCES, /* SCMI_ERR_ACCESS */ > + -ENOENT, /* SCMI_ERR_ENTRY */ > + -ERANGE, /* SCMI_ERR_RANGE */ > + -EBUSY, /* SCMI_ERR_BUSY */ > + -ECOMM, /* SCMI_ERR_COMMS */ > + -EIO, /* SCMI_ERR_GENERIC */ > + -EREMOTEIO, /* SCMI_ERR_HARDWARE */ > + -EPROTO, /* SCMI_ERR_PROTOCOL */ > +}; > + > +static inline int scmi_to_linux_errno(int errno) > +{ > + if (errno < SCMI_SUCCESS && errno > SCMI_ERR_MAX) > + return scmi_linux_errmap[-errno]; > + return -EIO; > +} > + > +/** > + * scmi_dump_header_dbg() - Helper to dump a message header. > + * > + * @dev: Device pointer corresponding to the SCMI entity > + * @hdr: pointer to header. > + */ > +static inline void scmi_dump_header_dbg(struct device *dev, > + struct scmi_msg_hdr *hdr) > +{ > + dev_dbg(dev, "Command ID: %x Sequence ID: %x Protocol: %x\n", > + hdr->id, hdr->seq, hdr->protocol_id); > +} > + > +static void scmi_fetch_response(struct scmi_xfer *xfer, > + struct scmi_shared_mem *mem) > +{ > + xfer->hdr.status = le32_to_cpu(*(__le32 *)mem->msg_payload); > + /* Skip the length of header and statues in payload area i.e 8 bytes*/ > + xfer->rx.len = min_t(size_t, xfer->rx.len, mem->length - 8); > + > + /* Take a copy to the rx buffer.. */ > + memcpy_fromio(xfer->rx.buf, mem->msg_payload + 4, xfer->rx.len); > +} > + > +/** > + * scmi_rx_callback() - mailbox client callback for receive messages > + * > + * @cl: client pointer > + * @m: mailbox message > + * > + * Processes one received message to appropriate transfer information and > + * signals completion of the transfer. > + * > + * NOTE: This function will be invoked in IRQ context, hence should be > + * as optimal as possible. > + */ > +static void scmi_rx_callback(struct mbox_client *cl, void *m) > +{ > + u16 xfer_id; > + struct scmi_xfer *xfer; > + struct scmi_info *info = client_to_scmi_info(cl); > + struct scmi_xfers_info *minfo = &info->minfo; > + struct device *dev = info->dev; > + struct scmi_shared_mem *mem = info->tx_payload; > + > + xfer_id = MSG_XTRACT_TOKEN(mem->msg_header); > + Don't we need to convert msg_header to cpu endian? > + /* > + * Are we even expecting this? > + */ > + if (!test_bit(xfer_id, minfo->xfer_alloc_table)) { > + dev_err(dev, "message for %d is not expected!\n", xfer_id); > + return; > + } > + > + xfer = &minfo->xfer_block[xfer_id]; > + > + scmi_dump_header_dbg(dev, &xfer->hdr); > + /* Is the message of valid length? */ > + if (xfer->rx.len > info->desc->max_msg_size) { > + dev_err(dev, "unable to handle %zu xfer(max %d)\n", > + xfer->rx.len, info->desc->max_msg_size); > + return; > + } > + > + scmi_fetch_response(xfer, mem); > + complete(&xfer->done); > +} > + > +/** > + * pack_scmi_header() - packs and returns 32-bit header > + * > + * @hdr: pointer to header containing all the information on message id, > + * protocol id and sequence id. > + */ > +static inline u32 pack_scmi_header(struct scmi_msg_hdr *hdr) > +{ > + return ((hdr->id & MSG_ID_MASK) << MSG_ID_SHIFT) | > + ((hdr->seq & MSG_TOKEN_ID_MASK) << MSG_TOKEN_ID_SHIFT) | > + ((hdr->protocol_id & MSG_PROTOCOL_ID_MASK) << MSG_PROTOCOL_ID_SHIFT); > +} > + > +/** > + * scmi_tx_prepare() - mailbox client callback to prepare for the transfer > + * > + * @cl: client pointer > + * @m: mailbox message > + * > + * This function prepares the shared memory which contains the header and the > + * payload. > + */ > +static void scmi_tx_prepare(struct mbox_client *cl, void *m) > +{ > + struct scmi_xfer *t = m; > + struct scmi_info *info = client_to_scmi_info(cl); > + struct scmi_shared_mem *mem = info->tx_payload; > + > + mem->channel_status = 0x0; /* Mark channel busy + clear error */ > + mem->flags = t->hdr.poll_completion ? 0 : SCMI_SHMEM_FLAG_INTR_ENABLED; > + mem->length = sizeof(mem->msg_header) + t->tx.len; > + mem->msg_header = cpu_to_le32(pack_scmi_header(&t->hdr)); > + if (t->tx.buf) > + memcpy_toio(mem->msg_payload, t->tx.buf, t->tx.len); > +} I thought every member of scmi_shared_mem should be in le, not just the header. If that is the case, why do mem->length and mem->flags not get converted to le? If it is not the case, should they be of type __le32? (same remark applies in scmi_fetch_response for mem->length) > + > +/** > + * scmi_one_xfer_get() - Allocate one message > + * > + * @handle: SCMI entity handle > + * > + * Helper function which is used by various command functions that are > + * exposed to clients of this driver for allocating a message traffic event. > + * > + * This function can sleep depending on pending requests already in the system > + * for the SCMI entity. Further, this also holds a spinlock to maintain > + * integrity of internal data structures. > + * > + * Return: 0 if all went fine, else corresponding error. > + */ > +static struct scmi_xfer *scmi_one_xfer_get(const struct scmi_handle *handle) > +{ > + u16 xfer_id; > + int ret, timeout; > + struct scmi_xfer *xfer; > + unsigned long flags, bit_pos; > + struct scmi_info *info = handle_to_scmi_info(handle); > + struct scmi_xfers_info *minfo = &info->minfo; > + > + /* > + * Ensure we have only controlled number of pending messages. > + * Ideally, we might just have to wait a single message, be > + * conservative and wait 5 times that.. > + */ > + timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms) * 5; > + ret = down_timeout(&minfo->sem_xfer_count, timeout); > + if (ret < 0) > + return ERR_PTR(ret); > + > + /* Keep the locked section as small as possible */ > + spin_lock_irqsave(&minfo->xfer_lock, flags); > + bit_pos = find_first_zero_bit(minfo->xfer_alloc_table, > + info->desc->max_msg); > + if (bit_pos == info->desc->max_msg) { > + spin_unlock_irqrestore(&minfo->xfer_lock, flags); > + return ERR_PTR(-ENOMEM); > + } > + set_bit(bit_pos, minfo->xfer_alloc_table); > + spin_unlock_irqrestore(&minfo->xfer_lock, flags); > + Is it needed to disable IRQs here? Since the callback called in IRQ context only reads the xfer_alloc_table without modification nor taking locks, can't we just do spin_lock/spin_unlock? > + xfer_id = bit_pos; > + > + xfer = &minfo->xfer_block[xfer_id]; > + xfer->hdr.seq = xfer_id; > + reinit_completion(&xfer->done); > + > + return xfer; > +} > + > +/** > + * scmi_one_xfer_put() - Release a message > + * > + * @minfo: transfer info pointer > + * @xfer: message that was reserved by scmi_one_xfer_get > + * > + * This holds a spinlock to maintain integrity of internal data structures. > + */ > +void scmi_one_xfer_put(const struct scmi_handle *handle, struct scmi_xfer *xfer) > +{ > + unsigned long flags; > + struct scmi_info *info = handle_to_scmi_info(handle); > + struct scmi_xfers_info *minfo = &info->minfo; > + > + /* > + * Keep the locked section as small as possible > + * NOTE: we might escape with smp_mb and no lock here.. > + * but just be conservative and symmetric. > + */ > + spin_lock_irqsave(&minfo->xfer_lock, flags); > + clear_bit(xfer->hdr.seq, minfo->xfer_alloc_table); > + spin_unlock_irqrestore(&minfo->xfer_lock, flags); > + Same question as before. Cheer, -- Julien Thierry -- 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 Julien, Thanks for reviewing this. On 05/09/17 11:03, Julien Thierry wrote: > Hi Sudeep, > > I am not sure what the patch does is correct when having a big endian > kernel dealing with scmi_shared_mem. Unless there is a reason not to > have SCMI with big endian kernel, please see remarks below. > No the intention at least is to have it working even with big endian kernel. I found couple of issues when testing after this was posted. They are already fixed in [1] [...] >> +/** >> + * scmi_rx_callback() - mailbox client callback for receive messages >> + * >> + * @cl: client pointer >> + * @m: mailbox message >> + * >> + * Processes one received message to appropriate transfer information >> and >> + * signals completion of the transfer. >> + * >> + * NOTE: This function will be invoked in IRQ context, hence should be >> + * as optimal as possible. >> + */ >> +static void scmi_rx_callback(struct mbox_client *cl, void *m) >> +{ >> + u16 xfer_id; >> + struct scmi_xfer *xfer; >> + struct scmi_info *info = client_to_scmi_info(cl); >> + struct scmi_xfers_info *minfo = &info->minfo; >> + struct device *dev = info->dev; >> + struct scmi_shared_mem *mem = info->tx_payload; >> + >> + xfer_id = MSG_XTRACT_TOKEN(mem->msg_header); >> + > > Don't we need to convert msg_header to cpu endian? > Indeed, already fixed as mentioned above. >> + /* >> + * Are we even expecting this? >> + */ >> + if (!test_bit(xfer_id, minfo->xfer_alloc_table)) { >> + dev_err(dev, "message for %d is not expected!\n", xfer_id); >> + return; >> + } >> + >> + xfer = &minfo->xfer_block[xfer_id]; >> + >> + scmi_dump_header_dbg(dev, &xfer->hdr); >> + /* Is the message of valid length? */ >> + if (xfer->rx.len > info->desc->max_msg_size) { >> + dev_err(dev, "unable to handle %zu xfer(max %d)\n", >> + xfer->rx.len, info->desc->max_msg_size); >> + return; >> + } >> + >> + scmi_fetch_response(xfer, mem); >> + complete(&xfer->done); >> +} >> + >> +/** >> + * pack_scmi_header() - packs and returns 32-bit header >> + * >> + * @hdr: pointer to header containing all the information on message id, >> + * protocol id and sequence id. >> + */ >> +static inline u32 pack_scmi_header(struct scmi_msg_hdr *hdr) >> +{ >> + return ((hdr->id & MSG_ID_MASK) << MSG_ID_SHIFT) | >> + ((hdr->seq & MSG_TOKEN_ID_MASK) << MSG_TOKEN_ID_SHIFT) | >> + ((hdr->protocol_id & MSG_PROTOCOL_ID_MASK) << >> MSG_PROTOCOL_ID_SHIFT); >> +} >> + >> +/** >> + * scmi_tx_prepare() - mailbox client callback to prepare for the >> transfer >> + * >> + * @cl: client pointer >> + * @m: mailbox message >> + * >> + * This function prepares the shared memory which contains the header >> and the >> + * payload. >> + */ >> +static void scmi_tx_prepare(struct mbox_client *cl, void *m) >> +{ >> + struct scmi_xfer *t = m; >> + struct scmi_info *info = client_to_scmi_info(cl); >> + struct scmi_shared_mem *mem = info->tx_payload; >> + >> + mem->channel_status = 0x0; /* Mark channel busy + clear error */ >> + mem->flags = t->hdr.poll_completion ? 0 : >> SCMI_SHMEM_FLAG_INTR_ENABLED; >> + mem->length = sizeof(mem->msg_header) + t->tx.len; >> + mem->msg_header = cpu_to_le32(pack_scmi_header(&t->hdr)); >> + if (t->tx.buf) >> + memcpy_toio(mem->msg_payload, t->tx.buf, t->tx.len); >> +} > > I thought every member of scmi_shared_mem should be in le, not just the > header. > If that is the case, why do mem->length and mem->flags not get converted > to le? > If it is not the case, should they be of type __le32? > > (same remark applies in scmi_fetch_response for mem->length) > Agreed and already fixed, sorry for that. I am planning to repost after 4.14-rc1. and hence waiting with fixes. >> + >> +/** >> + * scmi_one_xfer_get() - Allocate one message >> + * >> + * @handle: SCMI entity handle >> + * >> + * Helper function which is used by various command functions that are >> + * exposed to clients of this driver for allocating a message traffic >> event. >> + * >> + * This function can sleep depending on pending requests already in >> the system >> + * for the SCMI entity. Further, this also holds a spinlock to maintain >> + * integrity of internal data structures. >> + * >> + * Return: 0 if all went fine, else corresponding error. >> + */ >> +static struct scmi_xfer *scmi_one_xfer_get(const struct scmi_handle >> *handle) >> +{ >> + u16 xfer_id; >> + int ret, timeout; >> + struct scmi_xfer *xfer; >> + unsigned long flags, bit_pos; >> + struct scmi_info *info = handle_to_scmi_info(handle); >> + struct scmi_xfers_info *minfo = &info->minfo; >> + >> + /* >> + * Ensure we have only controlled number of pending messages. >> + * Ideally, we might just have to wait a single message, be >> + * conservative and wait 5 times that.. >> + */ >> + timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms) * 5; >> + ret = down_timeout(&minfo->sem_xfer_count, timeout); >> + if (ret < 0) >> + return ERR_PTR(ret); >> + >> + /* Keep the locked section as small as possible */ >> + spin_lock_irqsave(&minfo->xfer_lock, flags); >> + bit_pos = find_first_zero_bit(minfo->xfer_alloc_table, >> + info->desc->max_msg); >> + if (bit_pos == info->desc->max_msg) { >> + spin_unlock_irqrestore(&minfo->xfer_lock, flags); >> + return ERR_PTR(-ENOMEM); >> + } >> + set_bit(bit_pos, minfo->xfer_alloc_table); >> + spin_unlock_irqrestore(&minfo->xfer_lock, flags); >> + > > Is it needed to disable IRQs here? Since the callback called in IRQ > context only reads the xfer_alloc_table without modification nor taking > locks, can't we just do spin_lock/spin_unlock? > Yes we can for now. I started adding notification support where we may need to allocate(i.e. assign from the pre-allocated buffer) in IRQ context. I left it as is though I removed notifications as I haven't tested it. -- Regards, Sudeep [1] https://git.kernel.org/sudeep.holla/linux/h/for-list/arm_scmi -- 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 05/09/17 14:39, Julien Thierry wrote: [...] >> diff --git a/drivers/firmware/arm_scmi/driver.c >> b/drivers/firmware/arm_scmi/driver.c >> index 139d6980f270..601d0d7210d9 100644 >> --- a/drivers/firmware/arm_scmi/driver.c >> +++ b/drivers/firmware/arm_scmi/driver.c >> @@ -108,18 +108,22 @@ struct scmi_desc { >> * @dev: Device pointer >> * @desc: SoC description for this instance >> * @handle: Instance of SCMI handle to send to clients >> + * @version: SCMI revision information containing protocol version, >> + * implementation version and (sub-)vendor identification. >> * @cl: Mailbox Client >> * @tx_chan: Transmit mailbox channel >> * @rx_chan: Receive mailbox channel >> * @tx_payload: Transmit mailbox channel payload area >> * @rx_payload: Receive mailbox channel payload area >> * @minfo: Message info >> + * @protocols_imp: list of protocols implemented >> * @node: list head >> * @users: Number of users of this instance >> */ >> struct scmi_info { >> struct device *dev; >> const struct scmi_desc *desc; >> + struct scmi_revision_info version; >> struct scmi_handle handle; >> struct mbox_client cl; >> struct mbox_chan *tx_chan; >> @@ -127,6 +131,7 @@ struct scmi_info { >> void __iomem *tx_payload; >> void __iomem *rx_payload; >> struct scmi_xfers_info minfo; >> + u8 *protocols_imp; > > Both the base protocol and driver part rely on this being of size > MAX_PROTOCOLS_IMP (if existing). > > Could this be a "u8 protocols_imp[MAX_PROTOCOLS_IMP]" instead? Sounds better, will update accordingly. -- Regards, Sudeep -- 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 Sudeep, On 04/08/17 15:31, Sudeep Holla wrote: > The performance protocol is intended for the performance management of > group(s) of device(s) that run in the same performance domain. It > includes even the CPUs. A performance domain is defined by a set of > devices that always have to run at the same performance level. > For example, a set of CPUs that share a voltage domain, and have a > common frequency control, is said to be in the same performance domain. > > The commands in this protocol provide functionality to describe the > protocol version, describe various attribute flags, set and get the > performance level of a domain. It also supports discovery of the list > of performance levels supported by a performance domain, and the > properties of each performance level. > > This patch adds basic support for the performance protocol. > > Cc: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scmi/Makefile | 2 +- > drivers/firmware/arm_scmi/common.h | 1 + > drivers/firmware/arm_scmi/perf.c | 511 +++++++++++++++++++++++++++++++++++++ > include/linux/scmi_protocol.h | 31 +++ > 4 files changed, 544 insertions(+), 1 deletion(-) > create mode 100644 drivers/firmware/arm_scmi/perf.c > > diff --git a/drivers/firmware/arm_scmi/Makefile b/drivers/firmware/arm_scmi/Makefile > index 21d01d1d6b9c..159de726ee45 100644 > --- a/drivers/firmware/arm_scmi/Makefile > +++ b/drivers/firmware/arm_scmi/Makefile > @@ -1,2 +1,2 @@ > obj-$(CONFIG_ARM_SCMI_PROTOCOL) = arm_scmi.o > -arm_scmi-y = base.o driver.o > +arm_scmi-y = base.o driver.o perf.o > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > index e3fe5d9acc82..7473dfcad4ee 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -30,6 +30,7 @@ > #define PROTOCOL_REV_MAJOR(x) ((x) >> PROTOCOL_REV_MINOR_BITS) > #define PROTOCOL_REV_MINOR(x) ((x) & PROTOCOL_REV_MINOR_MASK) > #define MAX_PROTOCOLS_IMP 16 > +#define MAX_OPPS 16 > > enum scmi_std_protocol { > SCMI_PROTOCOL_BASE = 0x10, > diff --git a/drivers/firmware/arm_scmi/perf.c b/drivers/firmware/arm_scmi/perf.c > new file mode 100644 > index 000000000000..13d84d829201 > --- /dev/null > +++ b/drivers/firmware/arm_scmi/perf.c > @@ -0,0 +1,511 @@ > +/* > + * System Control and Management Interface (SCMI) Performance Protocol > + * > + * Copyright (C) 2017 ARM Ltd. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope 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. > + * > + * You should have received a copy of the GNU General Public License along > + * with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_opp.h> > +#include <linux/sort.h> > + > +#include "common.h" > + > +enum scmi_performance_protocol_cmd { > + PERF_DOMAIN_ATTRIBUTES = 0x3, > + PERF_DESCRIBE_LEVELS = 0x4, > + PERF_LIMITS_SET = 0x5, > + PERF_LIMITS_GET = 0x6, > + PERF_LEVEL_SET = 0x7, > + PERF_LEVEL_GET = 0x8, > + PERF_NOTIFY_LIMITS = 0x9, > + PERF_NOTIFY_LEVEL = 0xa, > +}; > + > +struct scmi_opp { > + u32 perf; > + u32 power; > + u32 trans_latency_us; > +}; > + > +struct scmi_msg_resp_perf_attributes { > + __le16 num_domains; > + __le16 flags; > +#define POWER_SCALE_IN_MILLIWATT(x) ((x) & BIT(0)) > + __le32 stats_addr_low; > + __le32 stats_addr_high; > + __le32 stats_size; > +}; > + > +struct scmi_msg_resp_perf_domain_attributes { > + __le32 flags; > +#define SUPPORTS_SET_LIMITS(x) ((x) & BIT(31)) > +#define SUPPORTS_SET_PERF_LVL(x) ((x) & BIT(30)) > +#define SUPPORTS_PERF_LIMIT_NOTIFY(x) ((x) & BIT(29)) > +#define SUPPORTS_PERF_LEVEL_NOTIFY(x) ((x) & BIT(28)) > + __le32 rate_limit_us; > + __le32 sustained_freq_khz; > + __le32 sustained_perf_level; > + u8 name[SCMI_MAX_STR_SIZE]; > +}; > + > +struct scmi_msg_perf_describe_levels { > + __le32 domain; > + __le32 level_index; > +}; > + > +struct scmi_perf_set_limits { > + __le32 domain; > + __le32 max_level; > + __le32 min_level; > +}; > + > +struct scmi_perf_get_limits { > + __le32 max_level; > + __le32 min_level; > +}; > + > +struct scmi_perf_set_level { > + __le32 domain; > + __le32 level; > +}; > + > +struct scmi_perf_notify_level_or_limits { > + __le32 domain; > + __le32 notify_enable; > +}; > + > +struct scmi_msg_resp_perf_describe_levels { > + __le16 num_returned; > + __le16 num_remaining; > + struct { > + __le32 perf_val; > + __le32 power; > + __le16 transition_latency_us; > + __le16 reserved; > + } opp[0]; > +}; > + > +struct perf_dom_info { > + bool set_limits; > + bool set_perf; > + bool perf_limit_notify; > + bool perf_level_notify; > + u32 opp_count; > + u32 sustained_freq_khz; > + u32 sustained_perf_level; > + u32 mult_factor; > + char name[SCMI_MAX_STR_SIZE]; > + struct scmi_opp opp[MAX_OPPS]; > +}; > + > +struct scmi_perf_info { > + int num_domains; > + bool power_scale_mw; > + u64 stats_addr; > + u32 stats_size; > + struct perf_dom_info *dom_info; > +}; > + > +static struct scmi_perf_info perf_info; > + > +static int scmi_perf_attributes_get(const struct scmi_handle *handle, > + struct scmi_perf_info *perf_info) > +{ > + int ret; > + struct scmi_xfer *t; > + struct scmi_msg_resp_perf_attributes *attr; > + > + ret = scmi_one_xfer_init(handle, PROTOCOL_ATTRIBUTES, > + SCMI_PROTOCOL_PERF, 0, sizeof(*attr), &t); > + if (ret) > + return ret; > + > + attr = t->rx.buf; > + > + ret = scmi_do_xfer(handle, t); > + if (!ret) { > + u16 flags = le16_to_cpu(attr->flags); > + > + perf_info->num_domains = le16_to_cpu(attr->num_domains); > + perf_info->power_scale_mw = POWER_SCALE_IN_MILLIWATT(flags); > + perf_info->stats_addr = le32_to_cpu(attr->stats_addr_low) | > + (u64)le32_to_cpu(attr->stats_addr_high) << 32; This seems odd, shouldn't it be the following? le64_to_cpu(attr->stats_addr_low | (__le64)attr->stats_addr_high << 32) Cheers, -- Julien Thierry -- 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 05/09/17 16:04, Julien Thierry wrote: > Hi Sudeep, > > On 04/08/17 15:31, Sudeep Holla wrote: >> The performance protocol is intended for the performance management of >> group(s) of device(s) that run in the same performance domain. It >> includes even the CPUs. A performance domain is defined by a set of >> devices that always have to run at the same performance level. >> For example, a set of CPUs that share a voltage domain, and have a >> common frequency control, is said to be in the same performance domain. >> >> The commands in this protocol provide functionality to describe the >> protocol version, describe various attribute flags, set and get the >> performance level of a domain. It also supports discovery of the list >> of performance levels supported by a performance domain, and the >> properties of each performance level. >> >> This patch adds basic support for the performance protocol. >> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/firmware/arm_scmi/Makefile | 2 +- >> drivers/firmware/arm_scmi/common.h | 1 + >> drivers/firmware/arm_scmi/perf.c | 511 >> +++++++++++++++++++++++++++++++++++++ >> include/linux/scmi_protocol.h | 31 +++ >> 4 files changed, 544 insertions(+), 1 deletion(-) >> create mode 100644 drivers/firmware/arm_scmi/perf.c >> >> diff --git a/drivers/firmware/arm_scmi/Makefile >> b/drivers/firmware/arm_scmi/Makefile >> index 21d01d1d6b9c..159de726ee45 100644 >> --- a/drivers/firmware/arm_scmi/Makefile >> +++ b/drivers/firmware/arm_scmi/Makefile >> @@ -1,2 +1,2 @@ >> obj-$(CONFIG_ARM_SCMI_PROTOCOL) = arm_scmi.o >> -arm_scmi-y = base.o driver.o >> +arm_scmi-y = base.o driver.o perf.o >> diff --git a/drivers/firmware/arm_scmi/common.h >> b/drivers/firmware/arm_scmi/common.h >> index e3fe5d9acc82..7473dfcad4ee 100644 >> --- a/drivers/firmware/arm_scmi/common.h >> +++ b/drivers/firmware/arm_scmi/common.h >> @@ -30,6 +30,7 @@ >> #define PROTOCOL_REV_MAJOR(x) ((x) >> PROTOCOL_REV_MINOR_BITS) >> #define PROTOCOL_REV_MINOR(x) ((x) & PROTOCOL_REV_MINOR_MASK) >> #define MAX_PROTOCOLS_IMP 16 >> +#define MAX_OPPS 16 >> enum scmi_std_protocol { >> SCMI_PROTOCOL_BASE = 0x10, >> diff --git a/drivers/firmware/arm_scmi/perf.c >> b/drivers/firmware/arm_scmi/perf.c >> new file mode 100644 >> index 000000000000..13d84d829201 >> --- /dev/null >> +++ b/drivers/firmware/arm_scmi/perf.c >> @@ -0,0 +1,511 @@ >> +/* >> + * System Control and Management Interface (SCMI) Performance Protocol >> + * >> + * Copyright (C) 2017 ARM Ltd. >> + * >> + * This program is free software; you can redistribute it and/or >> modify it >> + * under the terms and conditions of the GNU General Public License, >> + * version 2, as published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope 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. >> + * >> + * You should have received a copy of the GNU General Public License >> along >> + * with this program. If not, see <http://www.gnu.org/licenses/>. >> + */ >> + >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_opp.h> >> +#include <linux/sort.h> >> + >> +#include "common.h" >> + >> +enum scmi_performance_protocol_cmd { >> + PERF_DOMAIN_ATTRIBUTES = 0x3, >> + PERF_DESCRIBE_LEVELS = 0x4, >> + PERF_LIMITS_SET = 0x5, >> + PERF_LIMITS_GET = 0x6, >> + PERF_LEVEL_SET = 0x7, >> + PERF_LEVEL_GET = 0x8, >> + PERF_NOTIFY_LIMITS = 0x9, >> + PERF_NOTIFY_LEVEL = 0xa, >> +}; >> + >> +struct scmi_opp { >> + u32 perf; >> + u32 power; >> + u32 trans_latency_us; >> +}; >> + >> +struct scmi_msg_resp_perf_attributes { >> + __le16 num_domains; >> + __le16 flags; >> +#define POWER_SCALE_IN_MILLIWATT(x) ((x) & BIT(0)) >> + __le32 stats_addr_low; >> + __le32 stats_addr_high; >> + __le32 stats_size; >> +}; >> + >> +struct scmi_msg_resp_perf_domain_attributes { >> + __le32 flags; >> +#define SUPPORTS_SET_LIMITS(x) ((x) & BIT(31)) >> +#define SUPPORTS_SET_PERF_LVL(x) ((x) & BIT(30)) >> +#define SUPPORTS_PERF_LIMIT_NOTIFY(x) ((x) & BIT(29)) >> +#define SUPPORTS_PERF_LEVEL_NOTIFY(x) ((x) & BIT(28)) >> + __le32 rate_limit_us; >> + __le32 sustained_freq_khz; >> + __le32 sustained_perf_level; >> + u8 name[SCMI_MAX_STR_SIZE]; >> +}; >> + >> +struct scmi_msg_perf_describe_levels { >> + __le32 domain; >> + __le32 level_index; >> +}; >> + >> +struct scmi_perf_set_limits { >> + __le32 domain; >> + __le32 max_level; >> + __le32 min_level; >> +}; >> + >> +struct scmi_perf_get_limits { >> + __le32 max_level; >> + __le32 min_level; >> +}; >> + >> +struct scmi_perf_set_level { >> + __le32 domain; >> + __le32 level; >> +}; >> + >> +struct scmi_perf_notify_level_or_limits { >> + __le32 domain; >> + __le32 notify_enable; >> +}; >> + >> +struct scmi_msg_resp_perf_describe_levels { >> + __le16 num_returned; >> + __le16 num_remaining; >> + struct { >> + __le32 perf_val; >> + __le32 power; >> + __le16 transition_latency_us; >> + __le16 reserved; >> + } opp[0]; >> +}; >> + >> +struct perf_dom_info { >> + bool set_limits; >> + bool set_perf; >> + bool perf_limit_notify; >> + bool perf_level_notify; >> + u32 opp_count; >> + u32 sustained_freq_khz; >> + u32 sustained_perf_level; >> + u32 mult_factor; >> + char name[SCMI_MAX_STR_SIZE]; >> + struct scmi_opp opp[MAX_OPPS]; >> +}; >> + >> +struct scmi_perf_info { >> + int num_domains; >> + bool power_scale_mw; >> + u64 stats_addr; >> + u32 stats_size; >> + struct perf_dom_info *dom_info; >> +}; >> + >> +static struct scmi_perf_info perf_info; >> + >> +static int scmi_perf_attributes_get(const struct scmi_handle *handle, >> + struct scmi_perf_info *perf_info) >> +{ >> + int ret; >> + struct scmi_xfer *t; >> + struct scmi_msg_resp_perf_attributes *attr; >> + >> + ret = scmi_one_xfer_init(handle, PROTOCOL_ATTRIBUTES, >> + SCMI_PROTOCOL_PERF, 0, sizeof(*attr), &t); >> + if (ret) >> + return ret; >> + >> + attr = t->rx.buf; >> + >> + ret = scmi_do_xfer(handle, t); >> + if (!ret) { >> + u16 flags = le16_to_cpu(attr->flags); >> + >> + perf_info->num_domains = le16_to_cpu(attr->num_domains); >> + perf_info->power_scale_mw = POWER_SCALE_IN_MILLIWATT(flags); >> + perf_info->stats_addr = le32_to_cpu(attr->stats_addr_low) | >> + (u64)le32_to_cpu(attr->stats_addr_high) << 32; > > This seems odd, shouldn't it be the following? > le64_to_cpu(attr->stats_addr_low | (__le64)attr->stats_addr_high << 32) > After further reflexion, I think you are right. If I understood the specification, the address seems to be split into upper and lower 32bits and each one is stored as a uint32, which fits what you are doing to obtain the address. You can ignore my previous comment. -- Julien Thierry -- 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 Julien, On 05/09/17 16:56, Julien Thierry wrote: > > > On 05/09/17 16:04, Julien Thierry wrote: [...] >> >> This seems odd, shouldn't it be the following? >> le64_to_cpu(attr->stats_addr_low | (__le64)attr->stats_addr_high << 32) >> > > After further reflexion, I think you are right. If I understood the > specification, the address seems to be split into upper and lower 32bits > and each one is stored as a uint32, which fits what you are doing to > obtain the address. > > You can ignore my previous comment. > Sorry I am checking this bit late, you have figured out yourself already. All the 64 bit values need to be dealt similarly as SCMI specification restricts so. It's basically done so as the firmware running is mostly 32-bit little endian system. -- Regards, Sudeep -- 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 04/08/17 15:31, Sudeep Holla wrote: > Now that we have basic support for all the protocols in the > specification, let's probe them individually and initialise them. > > Cc: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> > --- > drivers/firmware/arm_scmi/common.h | 5 +++ > drivers/firmware/arm_scmi/driver.c | 80 +++++++++++++++++++++++++++++++++++++- > 2 files changed, 84 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/arm_scmi/common.h b/drivers/firmware/arm_scmi/common.h > index 7473dfcad4ee..d7c73a8d260b 100644 > --- a/drivers/firmware/arm_scmi/common.h > +++ b/drivers/firmware/arm_scmi/common.h > @@ -118,4 +118,9 @@ int scmi_version_get(const struct scmi_handle *h, u8 protocol, u32 *version); > void scmi_setup_protocol_implemented(const struct scmi_handle *handle, > u8 *prot_imp); > > +typedef int (*scmi_init_fn_t)(struct scmi_handle *); > int scmi_base_protocol_init(struct scmi_handle *h); > +int scmi_perf_protocol_init(struct scmi_handle *h); > +int scmi_sensors_protocol_init(struct scmi_handle *h); > +int scmi_power_protocol_init(struct scmi_handle *h); > +int scmi_clock_protocol_init(struct scmi_handle *h); > diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c > index 601d0d7210d9..6f31761043e2 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -157,6 +157,12 @@ struct scmi_shared_mem { > u8 msg_payload[0]; > }; > > +struct scmi_protocol_match { > + u8 protocol_id; > + scmi_init_fn_t fn; Could we call this "init" or "prot_init"? > + char name[32]; > +}; > + > static int scmi_linux_errmap[] = { > /* better than switch case as long as return value is continuous */ > 0, /* SCMI_SUCCESS */ > @@ -687,6 +693,41 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo) > return 0; > } > > +static const struct scmi_protocol_match scmi_protocols[] = { > + { > + .protocol_id = SCMI_PROTOCOL_PERF, > + .fn = scmi_perf_protocol_init, > + .name = "scmi-cpufreq", > + }, { > + .protocol_id = SCMI_PROTOCOL_CLOCK, > + .fn = scmi_clock_protocol_init, > + .name = "scmi-clocks", > + }, { > + .protocol_id = SCMI_PROTOCOL_POWER, > + .fn = scmi_power_protocol_init, > + .name = "scmi-power-domain", > + }, { > + .protocol_id = SCMI_PROTOCOL_SENSOR, > + .fn = scmi_sensors_protocol_init, > + .name = "scmi-hwmon", > + }, > + {} > +}; > + > +static const struct scmi_protocol_match *scmi_protocol_match_get(u8 protocol_id) > +{ > + int i; > + const struct scmi_protocol_match *match = NULL, *loop = scmi_protocols; > + > + for (i = 0; i < ARRAY_SIZE(scmi_protocols); i++, loop++) > + if (loop->protocol_id == protocol_id) { > + match = loop; > + break; > + } > + > + return match; > +} > + The "match" variable is not needed. We can just return "loop" in the if branch and return NULL at the end of the function. Unless we are following some coding standard that advises against that? > static int scmi_mailbox_check(struct device_node *np) > { > struct of_phandle_args arg; > @@ -778,7 +819,7 @@ static int scmi_probe(struct platform_device *pdev) > const struct scmi_desc *desc; > struct scmi_info *info; > struct device *dev = &pdev->dev; > - struct device_node *np = dev->of_node; > + struct device_node *child, *np = dev->of_node; > > /* Only mailbox method supported, check for the presence of one */ > if (scmi_mailbox_check(np)) { > @@ -817,6 +858,43 @@ static int scmi_probe(struct platform_device *pdev) > return ret; > } > > + for_each_available_child_of_node(np, child) { > + int init_ret; > + u32 prot_id; > + const struct scmi_protocol_match *match; > + > + if (of_property_read_u32(child, "reg", &prot_id)) > + continue; > + > + prot_id &= MSG_PROTOCOL_ID_MASK; > + > + if (!scmi_is_protocol_implemented(handle, prot_id)) { > + dev_err(dev, "SCMI protocol %d not implemented\n", > + prot_id); > + continue; > + } > + > + match = scmi_protocol_match_get(prot_id); > + if (match) { > + struct platform_device *cdev; > + > + cdev = of_platform_device_create(child, match->name, > + dev); > + if (!cdev) { > + dev_err(dev, "failed to create %s device\n", > + match->name); > + continue; > + } > + > + init_ret = match->fn(handle); > + if (init_ret) { > + dev_err(dev, "SCMI protocol %d init error %d\n", > + prot_id, init_ret); > + of_platform_device_destroy(&cdev->dev, NULL); > + } > + } > + } > + For the code in the "if (match)" branch, would it be better to have an inline function "scmi_create_protocol_device" or something with a name that fits? Also I was wondering, what is the difference between the protocol being implemented and finding the matching structure? Feels like there is a bit of redundancy. If the protocol is implemented but doesn't have a match structure does it just mean it doesn't need any specific initialization? Thanks, -- Julien Thierry -- 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 06/09/17 10:41, Julien Thierry wrote: > > > On 04/08/17 15:31, Sudeep Holla wrote: >> Now that we have basic support for all the protocols in the >> specification, let's probe them individually and initialise them. >> >> Cc: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >> --- >> drivers/firmware/arm_scmi/common.h | 5 +++ >> drivers/firmware/arm_scmi/driver.c | 80 >> +++++++++++++++++++++++++++++++++++++- >> 2 files changed, 84 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/arm_scmi/common.h >> b/drivers/firmware/arm_scmi/common.h >> index 7473dfcad4ee..d7c73a8d260b 100644 >> --- a/drivers/firmware/arm_scmi/common.h >> +++ b/drivers/firmware/arm_scmi/common.h >> @@ -118,4 +118,9 @@ int scmi_version_get(const struct scmi_handle *h, >> u8 protocol, u32 *version); >> void scmi_setup_protocol_implemented(const struct scmi_handle *handle, >> u8 *prot_imp); >> +typedef int (*scmi_init_fn_t)(struct scmi_handle *); >> int scmi_base_protocol_init(struct scmi_handle *h); >> +int scmi_perf_protocol_init(struct scmi_handle *h); >> +int scmi_sensors_protocol_init(struct scmi_handle *h); >> +int scmi_power_protocol_init(struct scmi_handle *h); >> +int scmi_clock_protocol_init(struct scmi_handle *h); >> diff --git a/drivers/firmware/arm_scmi/driver.c >> b/drivers/firmware/arm_scmi/driver.c >> index 601d0d7210d9..6f31761043e2 100644 >> --- a/drivers/firmware/arm_scmi/driver.c >> +++ b/drivers/firmware/arm_scmi/driver.c >> @@ -157,6 +157,12 @@ struct scmi_shared_mem { >> u8 msg_payload[0]; >> }; >> +struct scmi_protocol_match { >> + u8 protocol_id; >> + scmi_init_fn_t fn; > > Could we call this "init" or "prot_init"? > Done >> + char name[32]; >> +}; >> + >> static int scmi_linux_errmap[] = { >> /* better than switch case as long as return value is continuous */ >> 0, /* SCMI_SUCCESS */ >> @@ -687,6 +693,41 @@ static int scmi_xfer_info_init(struct scmi_info >> *sinfo) >> return 0; >> } >> +static const struct scmi_protocol_match scmi_protocols[] = { >> + { >> + .protocol_id = SCMI_PROTOCOL_PERF, >> + .fn = scmi_perf_protocol_init, >> + .name = "scmi-cpufreq", >> + }, { >> + .protocol_id = SCMI_PROTOCOL_CLOCK, >> + .fn = scmi_clock_protocol_init, >> + .name = "scmi-clocks", >> + }, { >> + .protocol_id = SCMI_PROTOCOL_POWER, >> + .fn = scmi_power_protocol_init, >> + .name = "scmi-power-domain", >> + }, { >> + .protocol_id = SCMI_PROTOCOL_SENSOR, >> + .fn = scmi_sensors_protocol_init, >> + .name = "scmi-hwmon", >> + }, >> + {} >> +}; >> + >> +static const struct scmi_protocol_match *scmi_protocol_match_get(u8 >> protocol_id) >> +{ >> + int i; >> + const struct scmi_protocol_match *match = NULL, *loop = >> scmi_protocols; >> + >> + for (i = 0; i < ARRAY_SIZE(scmi_protocols); i++, loop++) >> + if (loop->protocol_id == protocol_id) { >> + match = loop; >> + break; >> + } >> + >> + return match; >> +} >> + > > The "match" variable is not needed. We can just return "loop" in the if > branch and return NULL at the end of the function. Unless we are > following some coding standard that advises against that? > Agreed and fixed locally. >> static int scmi_mailbox_check(struct device_node *np) >> { >> struct of_phandle_args arg; >> @@ -778,7 +819,7 @@ static int scmi_probe(struct platform_device *pdev) >> const struct scmi_desc *desc; >> struct scmi_info *info; >> struct device *dev = &pdev->dev; >> - struct device_node *np = dev->of_node; >> + struct device_node *child, *np = dev->of_node; >> /* Only mailbox method supported, check for the presence of >> one */ >> if (scmi_mailbox_check(np)) { >> @@ -817,6 +858,43 @@ static int scmi_probe(struct platform_device *pdev) >> return ret; >> } >> + for_each_available_child_of_node(np, child) { >> + int init_ret; >> + u32 prot_id; >> + const struct scmi_protocol_match *match; >> + >> + if (of_property_read_u32(child, "reg", &prot_id)) >> + continue; >> + >> + prot_id &= MSG_PROTOCOL_ID_MASK; >> + >> + if (!scmi_is_protocol_implemented(handle, prot_id)) { >> + dev_err(dev, "SCMI protocol %d not implemented\n", >> + prot_id); >> + continue; >> + } >> + >> + match = scmi_protocol_match_get(prot_id); >> + if (match) { >> + struct platform_device *cdev; >> + >> + cdev = of_platform_device_create(child, match->name, >> + dev); >> + if (!cdev) { >> + dev_err(dev, "failed to create %s device\n", >> + match->name); >> + continue; >> + } >> + >> + init_ret = match->fn(handle); >> + if (init_ret) { >> + dev_err(dev, "SCMI protocol %d init error %d\n", >> + prot_id, init_ret); >> + of_platform_device_destroy(&cdev->dev, NULL); >> + } >> + } >> + } >> + > > For the code in the "if (match)" branch, would it be better to have an > inline function "scmi_create_protocol_device" or something with a name > that fits? > Done > Also I was wondering, what is the difference between the protocol being > implemented and finding the matching structure? Feels like there is a > bit of redundancy. If the protocol is implemented but doesn't have a > match structure does it just mean it doesn't need any specific > initialization? > Yes, that's the idea. One reason to have both check is backward compatibility. Suppose new protocols are implemented in the f/w but DT doesn't have them or know how to use it or there are no users of that feature, then skip initializing it. Similarly if DT is updated but not the firmware. -- Regards, Sudeep -- 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 06/09/17 14:31, Sudeep Holla wrote: > > > On 06/09/17 10:41, Julien Thierry wrote: >> >> >> On 04/08/17 15:31, Sudeep Holla wrote: >>> Now that we have basic support for all the protocols in the >>> specification, let's probe them individually and initialise them. >>> >>> Cc: Arnd Bergmann <arnd@arndb.de> >>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com> >>> --- >>> drivers/firmware/arm_scmi/common.h | 5 +++ >>> drivers/firmware/arm_scmi/driver.c | 80 >>> +++++++++++++++++++++++++++++++++++++- >>> 2 files changed, 84 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/firmware/arm_scmi/common.h >>> b/drivers/firmware/arm_scmi/common.h >>> index 7473dfcad4ee..d7c73a8d260b 100644 >>> --- a/drivers/firmware/arm_scmi/common.h >>> +++ b/drivers/firmware/arm_scmi/common.h >>> @@ -118,4 +118,9 @@ int scmi_version_get(const struct scmi_handle *h, >>> u8 protocol, u32 *version); >>> void scmi_setup_protocol_implemented(const struct scmi_handle *handle, >>> u8 *prot_imp); >>> +typedef int (*scmi_init_fn_t)(struct scmi_handle *); >>> int scmi_base_protocol_init(struct scmi_handle *h); >>> +int scmi_perf_protocol_init(struct scmi_handle *h); >>> +int scmi_sensors_protocol_init(struct scmi_handle *h); >>> +int scmi_power_protocol_init(struct scmi_handle *h); >>> +int scmi_clock_protocol_init(struct scmi_handle *h); >>> diff --git a/drivers/firmware/arm_scmi/driver.c >>> b/drivers/firmware/arm_scmi/driver.c >>> index 601d0d7210d9..6f31761043e2 100644 >>> --- a/drivers/firmware/arm_scmi/driver.c >>> +++ b/drivers/firmware/arm_scmi/driver.c >>> @@ -157,6 +157,12 @@ struct scmi_shared_mem { >>> u8 msg_payload[0]; >>> }; >>> +struct scmi_protocol_match { >>> + u8 protocol_id; >>> + scmi_init_fn_t fn; >> >> Could we call this "init" or "prot_init"? >> > > Done > >>> + char name[32]; >>> +}; >>> + >>> static int scmi_linux_errmap[] = { >>> /* better than switch case as long as return value is continuous */ >>> 0, /* SCMI_SUCCESS */ >>> @@ -687,6 +693,41 @@ static int scmi_xfer_info_init(struct scmi_info >>> *sinfo) >>> return 0; >>> } >>> +static const struct scmi_protocol_match scmi_protocols[] = { >>> + { >>> + .protocol_id = SCMI_PROTOCOL_PERF, >>> + .fn = scmi_perf_protocol_init, >>> + .name = "scmi-cpufreq", >>> + }, { >>> + .protocol_id = SCMI_PROTOCOL_CLOCK, >>> + .fn = scmi_clock_protocol_init, >>> + .name = "scmi-clocks", >>> + }, { >>> + .protocol_id = SCMI_PROTOCOL_POWER, >>> + .fn = scmi_power_protocol_init, >>> + .name = "scmi-power-domain", >>> + }, { >>> + .protocol_id = SCMI_PROTOCOL_SENSOR, >>> + .fn = scmi_sensors_protocol_init, >>> + .name = "scmi-hwmon", >>> + }, >>> + {} >>> +}; >>> + >>> +static const struct scmi_protocol_match *scmi_protocol_match_get(u8 >>> protocol_id) >>> +{ >>> + int i; >>> + const struct scmi_protocol_match *match = NULL, *loop = >>> scmi_protocols; >>> + >>> + for (i = 0; i < ARRAY_SIZE(scmi_protocols); i++, loop++) >>> + if (loop->protocol_id == protocol_id) { >>> + match = loop; >>> + break; >>> + } >>> + >>> + return match; >>> +} >>> + >> >> The "match" variable is not needed. We can just return "loop" in the if >> branch and return NULL at the end of the function. Unless we are >> following some coding standard that advises against that? >> > > Agreed and fixed locally. > >>> static int scmi_mailbox_check(struct device_node *np) >>> { >>> struct of_phandle_args arg; >>> @@ -778,7 +819,7 @@ static int scmi_probe(struct platform_device *pdev) >>> const struct scmi_desc *desc; >>> struct scmi_info *info; >>> struct device *dev = &pdev->dev; >>> - struct device_node *np = dev->of_node; >>> + struct device_node *child, *np = dev->of_node; >>> /* Only mailbox method supported, check for the presence of >>> one */ >>> if (scmi_mailbox_check(np)) { >>> @@ -817,6 +858,43 @@ static int scmi_probe(struct platform_device *pdev) >>> return ret; >>> } >>> + for_each_available_child_of_node(np, child) { >>> + int init_ret; >>> + u32 prot_id; >>> + const struct scmi_protocol_match *match; >>> + >>> + if (of_property_read_u32(child, "reg", &prot_id)) >>> + continue; >>> + >>> + prot_id &= MSG_PROTOCOL_ID_MASK; >>> + >>> + if (!scmi_is_protocol_implemented(handle, prot_id)) { >>> + dev_err(dev, "SCMI protocol %d not implemented\n", >>> + prot_id); >>> + continue; >>> + } >>> + >>> + match = scmi_protocol_match_get(prot_id); >>> + if (match) { >>> + struct platform_device *cdev; >>> + >>> + cdev = of_platform_device_create(child, match->name, >>> + dev); >>> + if (!cdev) { >>> + dev_err(dev, "failed to create %s device\n", >>> + match->name); >>> + continue; >>> + } >>> + >>> + init_ret = match->fn(handle); >>> + if (init_ret) { >>> + dev_err(dev, "SCMI protocol %d init error %d\n", >>> + prot_id, init_ret); >>> + of_platform_device_destroy(&cdev->dev, NULL); >>> + } >>> + } >>> + } >>> + >> >> For the code in the "if (match)" branch, would it be better to have an >> inline function "scmi_create_protocol_device" or something with a name >> that fits? >> > > Done > >> Also I was wondering, what is the difference between the protocol being >> implemented and finding the matching structure? Feels like there is a >> bit of redundancy. If the protocol is implemented but doesn't have a >> match structure does it just mean it doesn't need any specific >> initialization? >> > > Yes, that's the idea. One reason to have both check is backward > compatibility. Suppose new protocols are implemented in the f/w but DT > doesn't have them or know how to use it or there are no users of that > feature, then skip initializing it. Similarly if DT is updated but not > the firmware. > Oh I see, makes sense. Thanks for explaining. Cheers, -- Julien Thierry -- 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 all, Let me begin admitting that we are introducing yet another protocol to achieve same things as many existing protocols like ARM SCPI, TI SCI, QCOM RPM, Nvidia Tegra BPMP, and so on. All I can say is that this new ARM System Control and Management Interface(SCMI) is more flexible and easily extensible than any of the existing ones. Many vendors were involved in the making of this formal specification and is now officially published[1]. There is a strong trend in the industry to provide micro-controllers in systems to abstract various power, or other system management tasks. These controllers usually have similar interfaces, both in terms of the functions that are provided by them, and in terms of how requests are communicated to them. This specification is to standardise and avoid (any further) fragmentation in the design of such interface by various vendors. This patch set is intended to get feedback on the design and structure of the code. This is not complete and not fully tested due to non-availability of firmware with full feature set at this time. It currently doesn't support notification, asynchronous/delayed response, perf/power statistics region and sensor register region to name a few. I have borrowed some of the ideas of message allocation/management from TI SCI. I had previously posted RFC[2]. Changes: v1->v2: - Additional support for polling based DVFS and per protocol channels - Dependent drivers(clock, hwmon, cpufreq and power domains) - Various other review comments and issued found during testing addressed - Explicit binding for method dropped as even SMC based method are adviertised as mailbox RFC->v1: - Add generic mailbox binding for shared memory(Rob H) - Dropped compatibles per protocol(Suggested by Matt S) - Dropped lot of unnecessary pointer casting(Arnd B) - Dropped packing of structures(Arnd B) - Few other changes/additions based initial testing with firmware providing SCMI interface to OSPM -- Regards, Sudeep [1] http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/index.html [2] https://marc.info/?l=linux-kernel&m=149685193627620&w=2 [3] https://marc.info/?l=linux-arm-kernel&m=149849482623492&w=2 Sudeep Holla (18): dt-bindings: mailbox: add support for mailbox client shared memory dt-bindings: arm: add support for ARM System Control and Management Interface(SCMI) protocol firmware: arm_scmi: add basic driver infrastructure for SCMI firmware: arm_scmi: add common infrastructure and support for base protocol firmware: arm_scmi: add initial support for performance protocol firmware: arm_scmi: add initial support for clock protocol firmware: arm_scmi: add initial support for power protocol firmware: arm_scmi: add initial support for sensor protocol firmware: arm_scmi: probe and initialise all the supported protocols firmware: arm_scmi: add support for polling based SCMI transfers firmware: arm_scmi: add option for polling based performance domain operations firmware: arm_scmi: refactor in preparation to support per-protocol channels firmware: arm_scmi: add per-protocol channels support using idr objects firmware: arm_scmi: add device power domain support using genpd clk: add support for clocks provided by SCMI hwmon: add support for sensors exported via ARM SCMI cpufreq: add support for CPU DVFS based on SCMI message protocol cpufreq: scmi: add support for fast frequency switching Documentation/devicetree/bindings/arm/arm,scmi.txt | 174 ++++ .../devicetree/bindings/mailbox/mailbox.txt | 28 + drivers/clk/Kconfig | 10 + drivers/clk/Makefile | 1 + drivers/clk/clk-scmi.c | 216 +++++ drivers/cpufreq/Kconfig.arm | 11 + drivers/cpufreq/Makefile | 1 + drivers/cpufreq/scmi-cpufreq.c | 284 ++++++ drivers/firmware/Kconfig | 34 + drivers/firmware/Makefile | 1 + drivers/firmware/arm_scmi/Makefile | 3 + drivers/firmware/arm_scmi/base.c | 293 ++++++ drivers/firmware/arm_scmi/clock.c | 339 +++++++ drivers/firmware/arm_scmi/common.h | 126 +++ drivers/firmware/arm_scmi/driver.c | 992 +++++++++++++++++++++ drivers/firmware/arm_scmi/perf.c | 514 +++++++++++ drivers/firmware/arm_scmi/power.c | 242 +++++ drivers/firmware/arm_scmi/scmi_pm_domain.c | 134 +++ drivers/firmware/arm_scmi/sensors.c | 287 ++++++ drivers/hwmon/Kconfig | 12 + drivers/hwmon/Makefile | 1 + drivers/hwmon/scmi-hwmon.c | 261 ++++++ include/linux/scmi_protocol.h | 215 +++++ 23 files changed, 4179 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/arm,scmi.txt create mode 100644 drivers/clk/clk-scmi.c create mode 100644 drivers/cpufreq/scmi-cpufreq.c create mode 100644 drivers/firmware/arm_scmi/Makefile create mode 100644 drivers/firmware/arm_scmi/base.c create mode 100644 drivers/firmware/arm_scmi/clock.c create mode 100644 drivers/firmware/arm_scmi/common.h create mode 100644 drivers/firmware/arm_scmi/driver.c create mode 100644 drivers/firmware/arm_scmi/perf.c create mode 100644 drivers/firmware/arm_scmi/power.c create mode 100644 drivers/firmware/arm_scmi/scmi_pm_domain.c create mode 100644 drivers/firmware/arm_scmi/sensors.c create mode 100644 drivers/hwmon/scmi-hwmon.c create mode 100644 include/linux/scmi_protocol.h -- 2.7.4 -- 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