* Re: [PATCH v4] powernv, cpufreq: cpufreq driver for powernv platform
From: Viresh Kumar @ 2014-03-27 6:39 UTC (permalink / raw)
To: Gautham R. Shenoy, Rafael J. Wysocki
Cc: Linux PM list, linuxppc-dev@ozlabs.org, Anton Blanchard,
Srivatsa S. Bhat
In-Reply-To: <1395852947-22290-2-git-send-email-ego@linux.vnet.ibm.com>
Cc'ing Rafael.
On 26 March 2014 22:25, Gautham R. Shenoy <ego@linux.vnet.ibm.com> wrote:
> From: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
>
> Backend driver to dynamically set voltage and frequency on
> IBM POWER non-virtualized platforms. Power management SPRs
> are used to set the required PState.
>
> This driver works in conjunction with cpufreq governors
> like 'ondemand' to provide a demand based frequency and
> voltage setting on IBM POWER non-virtualized platforms.
>
> PState table is obtained from OPAL v3 firmware through device
> tree.
>
> powernv_cpufreq back-end driver would parse the relevant device-tree
> nodes and initialise the cpufreq subsystem on powernv platform.
>
> The code was originally written by svaidy@linux.vnet.ibm.com. Over
> time it was modified to accomodate bug-fixes as well as updates to the
> the cpu-freq core. Relevant portions of the change logs corresponding
> to those modifications are noted below:
>
> * The policy->cpus needs to be populated in a hotplug-invariant
> manner instead of using cpu_sibling_mask() which varies with
> cpu-hotplug. This is because the cpufreq core code copies this
> content into policy->related_cpus mask which is should not vary on
s/is /
> cpu-hotplug. [Authored by srivatsa.bhat@linux.vnet.ibm.com]
:)
> * On POWER systems, the CPU frequency is controlled at a core-level
> and hence we need to serialize so that only one of the threads in
> the core switches the core's frequency at a time. Introduce
> per-core locking to enable finer-grained synchronization and
> thereby enhance the speed and responsiveness of the cpufreq driver
> to varying workload demands.
>
> The design of per-core locking is very simple and
> straight-forward: we first define a Per-CPU lock and use the ones
> that belongs to the first thread sibling of the core.
>
> cpu_first_thread_sibling() macro is used to find the *common*
> lock for all thread siblings belonging to a core. [Authored by
> srivatsa.bhat@linux.vnet.ibm.com]
We don't need that after serialization patch of core is accepted. And it
should be accepted soon, in one form or other.
> * Create a helper routine that can return the cpu-frequency for the
> corresponding pstate_id. Also, cache the values of the pstate_max,
> pstate_min and pstate_nominal and nr_pstates in a static structure
> so that they can be reused in the future to perform any
> validations. [Authored by ego@linux.vnet.ibm.com]
>
> * Create a driver attribute named cpuinfo_nominal_freq which creates
> a sysfs read-only file named cpuinfo_nominal_freq. Export the
> frequency corresponding to the nominal_pstate through this
> interface.
>
> Nominal frequency is the highest non-turbo frequency for the
> platform. This is generally used for setting governor policies
> from user space for optimal energy efficiency. [Authored by
> ego@linux.vnet.ibm.com]
>
> * Implement a powernv_cpufreq_get(unsigned int cpu) method which will
> return the current operating frequency. Export this via the sysfs
> interface cpuinfo_cur_freq by setting powernv_cpufreq_driver.get to
> powernv_cpufreq_get(). [Authored by ego@linux.vnet.ibm.com]
>
> [Change log updated by ego@linux.vnet.ibm.com]
>
> Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> Signed-off-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> Signed-off-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
> Signed-off-by: Anton Blanchard <anton@samba.org>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> arch/powerpc/configs/pseries_defconfig | 1 +
> arch/powerpc/configs/pseries_le_defconfig | 1 +
> arch/powerpc/include/asm/reg.h | 4 +
> arch/powerpc/platforms/powernv/Kconfig | 6 +
> drivers/cpufreq/Kconfig.powerpc | 8 +
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/powernv-cpufreq.c | 372 ++++++++++++++++++++++++++++++
> 7 files changed, 393 insertions(+)
> create mode 100644 drivers/cpufreq/powernv-cpufreq.c
>
> diff --git a/arch/powerpc/configs/pseries_defconfig b/arch/powerpc/configs/pseries_defconfig
> index e9a8b4e..a285d44 100644
> --- a/arch/powerpc/configs/pseries_defconfig
> +++ b/arch/powerpc/configs/pseries_defconfig
> @@ -353,3 +353,4 @@ CONFIG_CRYPTO_DEV_NX_ENCRYPT=m
> CONFIG_VIRTUALIZATION=y
> CONFIG_KVM_BOOK3S_64=m
> CONFIG_KVM_BOOK3S_64_HV=y
> +CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y
> diff --git a/arch/powerpc/configs/pseries_le_defconfig b/arch/powerpc/configs/pseries_le_defconfig
> index 62771e0..47e6161 100644
> --- a/arch/powerpc/configs/pseries_le_defconfig
> +++ b/arch/powerpc/configs/pseries_le_defconfig
> @@ -350,3 +350,4 @@ CONFIG_CRYPTO_LZO=m
> # CONFIG_CRYPTO_ANSI_CPRNG is not set
> CONFIG_CRYPTO_DEV_NX=y
> CONFIG_CRYPTO_DEV_NX_ENCRYPT=m
> +CONFIG_CPU_FREQ_DEFAULT_GOV_ONDEMAND=y
don't know how Rafael want this, but probably this part could have gone
through ppc tree..
> diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h
> index 90c06ec..84f92ca 100644
> --- a/arch/powerpc/include/asm/reg.h
> +++ b/arch/powerpc/include/asm/reg.h
> @@ -271,6 +271,10 @@
> #define SPRN_HSRR1 0x13B /* Hypervisor Save/Restore 1 */
> #define SPRN_IC 0x350 /* Virtual Instruction Count */
> #define SPRN_VTB 0x351 /* Virtual Time Base */
> +#define SPRN_PMICR 0x354 /* Power Management Idle Control Reg */
> +#define SPRN_PMSR 0x355 /* Power Management Status Reg */
> +#define SPRN_PMCR 0x374 /* Power Management Control Register */
> +
> /* HFSCR and FSCR bit numbers are the same */
> #define FSCR_TAR_LG 8 /* Enable Target Address Register */
> #define FSCR_EBB_LG 7 /* Enable Event Based Branching */
> diff --git a/arch/powerpc/platforms/powernv/Kconfig b/arch/powerpc/platforms/powernv/Kconfig
> index 895e8a2..c252ee9 100644
> --- a/arch/powerpc/platforms/powernv/Kconfig
> +++ b/arch/powerpc/platforms/powernv/Kconfig
> @@ -11,6 +11,12 @@ config PPC_POWERNV
> select PPC_UDBG_16550
> select PPC_SCOM
> select ARCH_RANDOM
> + select CPU_FREQ
> + select CPU_FREQ_GOV_PERFORMANCE
> + select CPU_FREQ_GOV_POWERSAVE
> + select CPU_FREQ_GOV_USERSPACE
> + select CPU_FREQ_GOV_ONDEMAND
> + select CPU_FREQ_GOV_CONSERVATIVE
> default y
>
> config PPC_POWERNV_RTAS
> diff --git a/drivers/cpufreq/Kconfig.powerpc b/drivers/cpufreq/Kconfig.powerpc
> index ca0021a..72564b7 100644
> --- a/drivers/cpufreq/Kconfig.powerpc
> +++ b/drivers/cpufreq/Kconfig.powerpc
> @@ -54,3 +54,11 @@ config PPC_PASEMI_CPUFREQ
> help
> This adds the support for frequency switching on PA Semi
> PWRficient processors.
> +
> +config POWERNV_CPUFREQ
> + tristate "CPU frequency scaling for IBM POWERNV platform"
> + depends on PPC_POWERNV
> + default y
> + help
> + This adds support for CPU frequency switching on IBM POWERNV
> + platform
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 7494565..0dbb963 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -86,6 +86,7 @@ obj-$(CONFIG_PPC_CORENET_CPUFREQ) += ppc-corenet-cpufreq.o
> obj-$(CONFIG_CPU_FREQ_PMAC) += pmac32-cpufreq.o
> obj-$(CONFIG_CPU_FREQ_PMAC64) += pmac64-cpufreq.o
> obj-$(CONFIG_PPC_PASEMI_CPUFREQ) += pasemi-cpufreq.o
> +obj-$(CONFIG_POWERNV_CPUFREQ) += powernv-cpufreq.o
>
> ##################################################################################
> # Other platform drivers
> diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
> new file mode 100644
> index 0000000..b35865b
> --- /dev/null
> +++ b/drivers/cpufreq/powernv-cpufreq.c
> @@ -0,0 +1,372 @@
> +/*
> + * POWERNV cpufreq driver for the IBM POWER processors
> + *
> + * (C) Copyright IBM 2014
> + *
> + * Author: Vaidyanathan Srinivasan <svaidy at linux.vnet.ibm.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#define pr_fmt(fmt) "powernv-cpufreq: " fmt
> +
> +#include <linux/module.h>
> +#include <linux/cpufreq.h>
> +#include <linux/smp.h>
> +#include <linux/of.h>
> +#include <asm/cputhreads.h>
I thought I gave a comment on missing headers here?
> +/* Per-Core locking for frequency transitions */
> +static DEFINE_PER_CPU(struct mutex, freq_switch_lock);
> +
> +#define lock_core_freq(cpu) \
> + mutex_lock(&per_cpu(freq_switch_lock,\
> + cpu_first_thread_sibling(cpu)));
> +#define unlock_core_freq(cpu) \
> + mutex_unlock(&per_cpu(freq_switch_lock,\
> + cpu_first_thread_sibling(cpu)));
> +
> +#define POWERNV_MAX_PSTATES 256
> +
> +static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
> +static int powernv_pstate_ids[POWERNV_MAX_PSTATES+1];
I though we don't need this anymore? Either Rafael will take my patch as
is for the BOOST fixup or we will end up creating .isboost field in
struct cpufreq_frequency_table and so you could have used .driver_data here.
> +struct powernv_pstate_info {
> + int pstate_min_id;
> + int pstate_max_id;
> + int pstate_nominal_id;
> + int nr_pstates;
> +};
> +static struct powernv_pstate_info powernv_pstate_info;
Maybe write it as this (if you like :), as this is the only instance
of this struct):
Also, because 'powernv_pstate_info' is a local variable we can get rid of
powerenv_ from its name and name it just pstate_info. That will make
code shorter at many places and you may not be required to break
lines into two at some places. If you wish :)
+static struct powernv_pstate_info {
+ int pstate_min_id;
+ int pstate_max_id;
+ int pstate_nominal_id;
+ int nr_pstates;
+} powernv_pstate_info;
> +/*
> + * Initialize the freq table based on data obtained
> + * from the firmware passed via device-tree
> + */
> +static int init_powernv_pstates(void)
> +{
> + struct device_node *power_mgt;
> + int nr_pstates = 0;
> + int pstate_min, pstate_max, pstate_nominal;
> + const __be32 *pstate_ids, *pstate_freqs;
> + int i;
Can merge all the int definitions into a single line.
> + u32 len_ids, len_freqs;
> +
> + power_mgt = of_find_node_by_path("/ibm,opal/power-mgt");
> + if (!power_mgt) {
> + pr_warn("power-mgt node not found\n");
> + return -ENODEV;
> + }
> +
> + if (of_property_read_u32(power_mgt, "ibm,pstate-min", &pstate_min)) {
> + pr_warn("ibm,pstate-min node not found\n");
> + return -ENODEV;
> + }
> +
> + if (of_property_read_u32(power_mgt, "ibm,pstate-max", &pstate_max)) {
> + pr_warn("ibm,pstate-max node not found\n");
> + return -ENODEV;
> + }
Why do you need to get these from DT? And not find that yourself here instead?
> + if (of_property_read_u32(power_mgt, "ibm,pstate-nominal",
> + &pstate_nominal)) {
> + pr_warn("ibm,pstate-nominal not found\n");
> + return -ENODEV;
> + }
> + pr_info("cpufreq pstate min %d nominal %d max %d\n", pstate_min,
> + pstate_nominal, pstate_max);
> +
> + pstate_ids = of_get_property(power_mgt, "ibm,pstate-ids", &len_ids);
> + if (!pstate_ids) {
> + pr_warn("ibm,pstate-ids not found\n");
> + return -ENODEV;
> + }
> +
> + pstate_freqs = of_get_property(power_mgt, "ibm,pstate-frequencies-mhz",
> + &len_freqs);
> + if (!pstate_freqs) {
> + pr_warn("ibm,pstate-frequencies-mhz not found\n");
> + return -ENODEV;
> + }
> +
> + WARN_ON(len_ids != len_freqs);
> + nr_pstates = min(len_ids, len_freqs) / sizeof(u32);
> + if (!nr_pstates) {
> + WARN_ON(1);
> + return -ENODEV;
> + }
Maybe like this:
+ if (WARN_ON(!nr_pstates))
+ return -ENODEV;
> + pr_debug("NR PStates %d\n", nr_pstates);
> + for (i = 0; i < nr_pstates; i++) {
> + u32 id = be32_to_cpu(pstate_ids[i]);
> + u32 freq = be32_to_cpu(pstate_freqs[i]);
> +
> + pr_debug("PState id %d freq %d MHz\n", id, freq);
> + powernv_freqs[i].driver_data = i;
Looks like more than one comments aren't addressed :(
You can use this field for your id. And even if you couldn't have
done that, you don't need to initialize this field at all..
> + powernv_freqs[i].frequency = freq * 1000; /* kHz */
> + powernv_pstate_ids[i] = id;
> + }
> + /* End of list marker entry */
> + powernv_freqs[i].frequency = CPUFREQ_TABLE_END;
> +
> + powernv_pstate_info.pstate_min_id = pstate_min;
> + powernv_pstate_info.pstate_max_id = pstate_max;
> + powernv_pstate_info.pstate_nominal_id = pstate_nominal;
> + powernv_pstate_info.nr_pstates = nr_pstates;
> +
> + return 0;
> +}
> +
> +/*
> + * Returns the cpu frequency corresponding to the pstate_id.
> + */
Maybe:
+/* Returns the cpu frequency corresponding to the pstate_id. */
> +static unsigned int pstate_id_to_freq(int pstate_id)
> +{
> + int i;
> +
> + i = powernv_pstate_info.pstate_max_id - pstate_id;
It looks like these ids would always be contiguous? In that case
it would be better if you can mention this property at the top of this
file in some comment. So, that new people can understand things
quickly.
> + BUG_ON(i >= powernv_pstate_info.nr_pstates || i < 0);
> + WARN_ON(powernv_pstate_ids[i] != pstate_id);
Do you really want it? We have already confirmed that 'i' is
within limits.
> + return powernv_freqs[i].frequency;
> +}
> +
> +/*
> + * cpuinfo_nominal_freq_show - Show the nominal CPU frequency as indicated by
> + * the firmware
> + */
> +static ssize_t cpuinfo_nominal_freq_show(struct cpufreq_policy *policy,
> + char *buf)
> +{
> + return sprintf(buf, "%u\n",
> + pstate_id_to_freq(powernv_pstate_info.pstate_nominal_id));
> +}
> +
> +struct freq_attr cpufreq_freq_attr_cpuinfo_nominal_freq =
> + __ATTR_RO(cpuinfo_nominal_freq);
> +
> +static struct freq_attr *powernv_cpu_freq_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + &cpufreq_freq_attr_cpuinfo_nominal_freq,
> + NULL,
> +};
> +
> +/* Helper routines */
> +
> +/* Access helpers to power mgt SPR */
> +
> +static inline unsigned long get_pmspr(unsigned long sprn)
> +{
> + switch (sprn) {
> + case SPRN_PMCR:
> + return mfspr(SPRN_PMCR);
> +
> + case SPRN_PMICR:
> + return mfspr(SPRN_PMICR);
> +
> + case SPRN_PMSR:
> + return mfspr(SPRN_PMSR);
> + }
> + BUG();
> +}
> +
> +static inline void set_pmspr(unsigned long sprn, unsigned long val)
> +{
> + switch (sprn) {
> + case SPRN_PMCR:
> + mtspr(SPRN_PMCR, val);
> + return;
> +
> + case SPRN_PMICR:
> + mtspr(SPRN_PMICR, val);
> + return;
> +
> + case SPRN_PMSR:
> + mtspr(SPRN_PMSR, val);
> + return;
> + }
> + BUG();
> +}
> +
> +/*
> + * Use objects of this type to query/update
> + * pstates on a remote cpu via smp_call_function.
> + */
> +struct powernv_smp_call_data {
> + unsigned int freq;
> + int pstate_id;
> +};
> +
> +/*
> + * powernv_read_cpu_freq: Reads the current frequency on this cpu.
> + *
> + * Called via smp_call_function.
> + *
> + * Note: The caller of the smp_call_function should pass an argument of
> + * the type 'struct powernv_smp_call_data *' along with this function.
> + *
> + * The current frequency on this cpu will be returned via
> + * ((struct powernv_smp_call_data *)arg)->freq;
> + */
> +static void powernv_read_cpu_freq(void *arg)
> +{
> + unsigned long pmspr_val;
> + s8 local_pstate_id;
> + struct powernv_smp_call_data *freq_data;
> +
> + freq_data = (struct powernv_smp_call_data *)arg;
don't need casting here ?
> +
> + pmspr_val = get_pmspr(SPRN_PMSR);
> +
> + /*
> + * The local pstate id corresponds bits 48..55 in the PMSR.
> + * Note: Watch out for the sign!
> + */
> + local_pstate_id = (pmspr_val >> 48) & 0xFF;
> + freq_data->pstate_id = local_pstate_id;
> + freq_data->freq = pstate_id_to_freq(freq_data->pstate_id);
> +
> + pr_debug("cpu %d pmsr %lx pstate_id %d frequency %d kHz \n",
> + smp_processor_id(), pmspr_val, freq_data->pstate_id,
s/smp_processor_id/raw_smp_processor_id ?
> + freq_data->freq);
> +}
> +
> +/*
> + * powernv_cpufreq_get: Returns the cpu frequency as reported by the
> + * firmware for 'cpu'. This value is reported through the sysfs file
> + * cpuinfo_cur_freq.
> + */
> +unsigned int powernv_cpufreq_get(unsigned int cpu)
> +{
> + struct powernv_smp_call_data freq_data;
> +
> + smp_call_function_any(cpu_sibling_mask(cpu), powernv_read_cpu_freq,
> + &freq_data, 1);
> +
> + return freq_data.freq;
> +}
> +
> +/*
> + * set_pstate: Sets the frequency on this cpu.
> + *
> + * This is called via an smp_call_function.
> + *
> + * The caller must ensure that freq_data is of the type
> + * (struct powernv_smp_call_data *) and the pstate_id which needs to be set
> + * on this cpu should be present in freq_data->pstate_id.
> + */
> +static void set_pstate(void *freq_data)
> +{
> + unsigned long val;
> + unsigned long pstate_ul =
> + ((struct powernv_smp_call_data *) freq_data)->pstate_id;
> +
> + val = get_pmspr(SPRN_PMCR);
> + val = val & 0x0000ffffffffffffULL;
> +
> + pstate_ul = pstate_ul & 0xFF;
> +
> + /* Set both global(bits 56..63) and local(bits 48..55) PStates */
> + val = val | (pstate_ul << 56) | (pstate_ul << 48);
> +
> + pr_debug("Setting cpu %d pmcr to %016lX\n", smp_processor_id(), val);
s/smp_processor_id/raw_smp_processor_id ? At other places as well.
> + set_pmspr(SPRN_PMCR, val);
> +}
> +
> +/*
> + * powernv_set_freq: Sets the frequency corresponding to the cpufreq
> + * table entry indexed by new_index on the cpus in the mask 'cpus'
Rafael doesn't like CPUs to be written as cpus.. I got this comment long
back :) (Applicable only for comments and logs)
> + */
> +static int powernv_set_freq(cpumask_var_t cpus, unsigned int new_index)
Why do you want to keep this routine separately? Why not have a single routine
powernv_cpufreq_target_index() ?
> +{
> + struct powernv_smp_call_data freq_data;
> +
> + freq_data.pstate_id = powernv_pstate_ids[new_index];
> +
> + /*
> + * Use smp_call_function to send IPI and execute the
> + * mtspr on target cpu. We could do that without IPI
> + * if current CPU is within policy->cpus (core)
> + */
> + smp_call_function_any(cpus, set_pstate, &freq_data, 1);
Not sure how smp_call_function_any() behaves but wouldn't it be
a good optimization if you can check if raw_smp_processor_id()
returns one of the CPUs from 'cpus'? And in that case don't
shoot an IPI.
Same for the get part as well.
But then I looked at the implementation of these routines and found
they already have this optimization in :) .. But with overhead of few
locks and disable_preempt() :(
> + return 0;
> +}
> +
> +static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)
> +{
> + int base, i;
> +
> + base = cpu_first_thread_sibling(policy->cpu);
> +
> + for (i = 0; i < threads_per_core; i++)
> + cpumask_set_cpu(base + i, policy->cpus);
> +
> + policy->cpuinfo.transition_latency = 25000;
> + policy->cur = powernv_freqs[0].frequency;
How can you be so sure? Also clock is doing this just after calling init()
and so you can just remove it :)
> + return cpufreq_table_validate_and_show(policy, powernv_freqs);
> +}
> +
> +static int powernv_cpufreq_verify(struct cpufreq_policy *policy)
> +{
> + return cpufreq_generic_frequency_table_verify(policy);
> +}
> +
> +static int powernv_cpufreq_target_index(struct cpufreq_policy *policy,
> + unsigned int new_index)
> +{
> + int rc;
> +
> + lock_core_freq(policy->cpu);
> + rc = powernv_set_freq(policy->cpus, new_index);
> + unlock_core_freq(policy->cpu);
> +
> + return rc;
> +}
> +
> +static struct cpufreq_driver powernv_cpufreq_driver = {
> + .name = "powernv-cpufreq",
> + .flags = CPUFREQ_CONST_LOOPS,
> + .init = powernv_cpufreq_cpu_init,
> + .verify = powernv_cpufreq_verify,
Can do this instead:
+ .verify = cpufreq_generic_frequency_table_verify,
> + .target_index = powernv_cpufreq_target_index,
> + .get = powernv_cpufreq_get,
> + .attr = powernv_cpu_freq_attr,
> +};
> +
> +static int __init powernv_cpufreq_init(void)
> +{
> + int cpu, rc = 0;
> +
> + /* Discover pstates from device tree and init */
> + rc = init_powernv_pstates();
> + if (rc) {
> + pr_info("powernv-cpufreq disabled\n");
> + return rc;
> + }
> +
> + /* Init per-core mutex */
> + for_each_possible_cpu(cpu)
> + mutex_init(&per_cpu(freq_switch_lock, cpu));
> +
> + return cpufreq_register_driver(&powernv_cpufreq_driver);
> +}
> +module_init(powernv_cpufreq_init);
> +
> +static void __exit powernv_cpufreq_exit(void)
> +{
> + cpufreq_unregister_driver(&powernv_cpufreq_driver);
> +}
> +module_exit(powernv_cpufreq_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Vaidyanathan Srinivasan <svaidy at linux.vnet.ibm.com>");
^ permalink raw reply
* Re: [PATCH v4] powernv: Dynamic Frequency Scaling Enablement
From: Benjamin Herrenschmidt @ 2014-03-27 6:28 UTC (permalink / raw)
To: Viresh Kumar
Cc: Gautham R. Shenoy, Linux PM list, linuxppc-dev@ozlabs.org,
Anton Blanchard, Srivatsa S. Bhat
In-Reply-To: <CAKohpom-mnOXpOUChasOMVTrRGknvZNA_bOcq6ZfGTRmUM5W9A@mail.gmail.com>
On Thu, 2014-03-27 at 11:08 +0530, Viresh Kumar wrote:
> On 26 March 2014 22:25, Gautham R. Shenoy <ego@linux.vnet.ibm.com> wrote:
> > This is the v4 of the patchset to enable Dynamic Frequency Scaling
> > on IBM PowerNV Platforms. I have incorporated the review comments
> > from the previous version (can be found at [1]).
>
> I wouldn't have added a cover-letter if I only have a single patch to
> send. Would have rather used the not-to-be-commited field of the
> patch instead.
Any other comment ? :-)
Because it depends on some other stuff in -next, I think it's best if
this gets merged via Rafael's tree, unless someone can point me at
the necessary pre-requisites in the form of a topic branch I can
pull in powerpc along with this.
Cheers,
Ben.
^ permalink raw reply
* [PATCH] phy/at8031: enable at8031 to work on interrupt mode
From: Zhao Qiang @ 2014-03-27 6:18 UTC (permalink / raw)
To: linuxppc-dev, netdev, B07421
Cc: mugunthanvnm, Zhao Qiang, linux-kernel, helmut.schaa, zonque,
R63061, davem
The at8031 can work on polling mode and interrupt mode.
Add ack_interrupt and config intr funcs to enable
interrupt mode for it.
Signed-off-by: Zhao Qiang <B45475@freescale.com>
---
drivers/net/phy/at803x.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index bc71947..d034ef5 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -27,6 +27,9 @@
#define AT803X_MMD_ACCESS_CONTROL 0x0D
#define AT803X_MMD_ACCESS_CONTROL_DATA 0x0E
#define AT803X_FUNC_DATA 0x4003
+#define AT803X_INER 0x0012
+#define AT803X_INER_INIT 0xec00
+#define AT803X_INSR 0x0013
#define AT803X_DEBUG_ADDR 0x1D
#define AT803X_DEBUG_DATA 0x1E
#define AT803X_DEBUG_SYSTEM_MODE_CTRL 0x05
@@ -191,6 +194,31 @@ static int at803x_config_init(struct phy_device *phydev)
return 0;
}
+static int at803x_ack_interrupt(struct phy_device *phydev)
+{
+ int err;
+
+ err = phy_read(phydev, AT803X_INSR);
+
+ return (err < 0) ? err : 0;
+}
+
+static int at803x_config_intr(struct phy_device *phydev)
+{
+ int err;
+ int value;
+
+ value = phy_read(phydev, AT803X_INER);
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ err = phy_write(phydev, AT803X_INER,
+ (value | AT803X_INER_INIT));
+ else
+ err = phy_write(phydev, AT803X_INER, value);
+
+ return err;
+}
+
static struct phy_driver at803x_driver[] = {
{
/* ATHEROS 8035 */
@@ -240,6 +268,8 @@ static struct phy_driver at803x_driver[] = {
.flags = PHY_HAS_INTERRUPT,
.config_aneg = genphy_config_aneg,
.read_status = genphy_read_status,
+ .ack_interrupt = &at803x_ack_interrupt,
+ .config_intr = &at803x_config_intr,
.driver = {
.owner = THIS_MODULE,
},
--
1.8.5
^ permalink raw reply related
* Re: [PATCH 1/1] mm: move FAULT_AROUND_ORDER to arch/
From: Madhavan Srinivasan @ 2014-03-27 6:20 UTC (permalink / raw)
To: Kirill A. Shutemov
Cc: linux-arch, riel, rusty, peterz, x86, linux-kernel, linux-mm, ak,
paulus, mgorman, akpm, linuxppc-dev, mingo, kirill.shutemov
In-Reply-To: <20140325173605.GA21411@node.dhcp.inet.fi>
On Tuesday 25 March 2014 11:06 PM, Kirill A. Shutemov wrote:
> On Tue, Mar 25, 2014 at 12:20:15PM +0530, Madhavan Srinivasan wrote:
>> Kirill A. Shutemov with the commit 96bacfe542 introduced
>> vm_ops->map_pages() for mapping easy accessible pages around
>> fault address in hope to reduce number of minor page faults.
>> Based on his workload runs, suggested FAULT_AROUND_ORDER
>> (knob to control the numbers of pages to map) is 4.
>>
>> This patch moves the FAULT_AROUND_ORDER macro to arch/ for
>> architecture maintainers to decide on suitable FAULT_AROUND_ORDER
>> value based on performance data for that architecture.
>>
>> Signed-off-by: Madhavan Srinivasan <maddy@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/include/asm/pgtable.h | 6 ++++++
>> arch/x86/include/asm/pgtable.h | 5 +++++
>> include/asm-generic/pgtable.h | 10 ++++++++++
>> mm/memory.c | 2 --
>> 4 files changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
>> index 3ebb188..9fcbd48 100644
>> --- a/arch/powerpc/include/asm/pgtable.h
>> +++ b/arch/powerpc/include/asm/pgtable.h
>> @@ -19,6 +19,12 @@ struct mm_struct;
>> #endif
>>
>> /*
>> + * With a few real world workloads that were run,
>> + * the performance data showed that a value of 3 is more advantageous.
>> + */
>> +#define FAULT_AROUND_ORDER 3
>> +
>> +/*
>> * We save the slot number & secondary bit in the second half of the
>> * PTE page. We use the 8 bytes per each pte entry.
>> */
>> diff --git a/arch/x86/include/asm/pgtable.h b/arch/x86/include/asm/pgtable.h
>> index 938ef1d..8387a65 100644
>> --- a/arch/x86/include/asm/pgtable.h
>> +++ b/arch/x86/include/asm/pgtable.h
>> @@ -7,6 +7,11 @@
>> #include <asm/pgtable_types.h>
>>
>> /*
>> + * Based on Kirill's test results, fault around order is set to 4
>> + */
>> +#define FAULT_AROUND_ORDER 4
>> +
>> +/*
>> * Macro to mark a page protection value as UC-
>> */
>> #define pgprot_noncached(prot) \
>> diff --git a/include/asm-generic/pgtable.h b/include/asm-generic/pgtable.h
>> index 1ec08c1..62f7f07 100644
>> --- a/include/asm-generic/pgtable.h
>> +++ b/include/asm-generic/pgtable.h
>> @@ -7,6 +7,16 @@
>> #include <linux/mm_types.h>
>> #include <linux/bug.h>
>>
>> +
>> +/*
>> + * Fault around order is a control knob to decide the fault around pages.
>> + * Default value is set to 0UL (disabled), but the arch can override it as
>> + * desired.
>> + */
>> +#ifndef FAULT_AROUND_ORDER
>> +#define FAULT_AROUND_ORDER 0UL
>> +#endif
>
> FAULT_AROUND_ORDER == 0 case should be handled separately in
> do_read_fault(): no reason to go to do_fault_around() if we are going to
> fault in only one page.
>
ok agreed. I am thinking of adding FAULT_AROUND_ORDER check with
map_pages check in the do_read_fault. Kindly share your thoughts.
With regards
Maddy
^ permalink raw reply
* Re: [PATCH 4/4] powerpc/powernv: Fix little endian issues OPAL error log code
From: Stewart Smith @ 2014-03-27 6:17 UTC (permalink / raw)
To: Anton Blanchard, benh, paulus; +Cc: linuxppc-dev
In-Reply-To: <20140327162056.5a57c4c9@kryten>
Anton Blanchard <anton@samba.org> writes:
> Fix little endian issues with the OPAL error log code.
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
Reviewed-by: Stewart Smith <stewart@linux.vnet.ibm.com>
Do we also need any magic for the getting of error logs themselves?
You may want to check the md5 of the logs themselves on be and le.
> ---
>
> Index: b/arch/powerpc/platforms/powernv/opal-elog.c
> ===================================================================
> --- a/arch/powerpc/platforms/powernv/opal-elog.c
> +++ b/arch/powerpc/platforms/powernv/opal-elog.c
> @@ -243,18 +243,25 @@ static struct elog_obj *create_elog_obj(
>
> static void elog_work_fn(struct work_struct *work)
> {
> + __be64 size;
> + __be64 id;
> + __be64 type;
> uint64_t elog_size;
> uint64_t log_id;
> uint64_t elog_type;
> int rc;
> char name[2+16+1];
>
> - rc = opal_get_elog_size(&log_id, &elog_size, &elog_type);
> + rc = opal_get_elog_size(&id, &size, &type);
> if (rc != OPAL_SUCCESS) {
> pr_err("ELOG: Opal log read failed\n");
> return;
> }
>
> + elog_size = be64_to_cpu(size);
> + log_id = be64_to_cpu(id);
> + elog_type = be64_to_cpu(type);
> +
> BUG_ON(elog_size > OPAL_MAX_ERRLOG_SIZE);
>
> if (elog_size >= OPAL_MAX_ERRLOG_SIZE)
> Index: b/arch/powerpc/include/asm/opal.h
> ===================================================================
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -851,7 +851,7 @@ int64_t opal_lpc_read(uint32_t chip_id,
> uint32_t addr, __be32 *data, uint32_t sz);
>
> int64_t opal_read_elog(uint64_t buffer, uint64_t size, uint64_t log_id);
> -int64_t opal_get_elog_size(uint64_t *log_id, uint64_t *size, uint64_t *elog_type);
> +int64_t opal_get_elog_size(__be64 *log_id, __be64 *size, __be64 *elog_type);
> int64_t opal_write_elog(uint64_t buffer, uint64_t size, uint64_t offset);
> int64_t opal_send_ack_elog(uint64_t log_id);
> void opal_resend_pending_logs(void);
^ permalink raw reply
* Re: [PATCH 1/4] powerpc/powernv: Use uint64_t instead of size_t in OPAL APIs
From: Stewart Smith @ 2014-03-27 5:54 UTC (permalink / raw)
To: Anton Blanchard, benh, paulus; +Cc: linuxppc-dev
In-Reply-To: <20140327161849.791432d0@kryten>
Anton Blanchard <anton@samba.org> writes:
> Using size_t in our APIs is asking for trouble, especially
> when some OPAL calls use size_t pointers.
>
> Signed-off-by: Anton Blanchard <anton@samba.org>
Reviewed-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> ---
>
> Index: b/arch/powerpc/include/asm/opal.h
> ===================================================================
> --- a/arch/powerpc/include/asm/opal.h
> +++ b/arch/powerpc/include/asm/opal.h
> @@ -850,8 +850,8 @@ int64_t opal_lpc_write(uint32_t chip_id,
> int64_t opal_lpc_read(uint32_t chip_id, enum OpalLPCAddressType addr_type,
> uint32_t addr, __be32 *data, uint32_t sz);
>
> -int64_t opal_read_elog(uint64_t buffer, size_t size, uint64_t log_id);
> -int64_t opal_get_elog_size(uint64_t *log_id, size_t *size, uint64_t *elog_type);
> +int64_t opal_read_elog(uint64_t buffer, uint64_t size, uint64_t log_id);
> +int64_t opal_get_elog_size(uint64_t *log_id, uint64_t *size, uint64_t *elog_type);
> int64_t opal_write_elog(uint64_t buffer, uint64_t size, uint64_t offset);
> int64_t opal_send_ack_elog(uint64_t log_id);
> void opal_resend_pending_logs(void);
> @@ -866,13 +866,13 @@ int64_t opal_dump_read(uint32_t dump_id,
> int64_t opal_dump_ack(uint32_t dump_id);
> int64_t opal_dump_resend_notification(void);
>
> -int64_t opal_get_msg(uint64_t buffer, size_t size);
> -int64_t opal_check_completion(uint64_t buffer, size_t size, uint64_t token);
> +int64_t opal_get_msg(uint64_t buffer, uint64_t size);
> +int64_t opal_check_completion(uint64_t buffer, uint64_t size, uint64_t token);
> int64_t opal_sync_host_reboot(void);
> int64_t opal_get_param(uint64_t token, uint32_t param_id, uint64_t buffer,
> - size_t length);
> + uint64_t length);
> int64_t opal_set_param(uint64_t token, uint32_t param_id, uint64_t buffer,
> - size_t length);
> + uint64_t length);
> int64_t opal_sensor_read(uint32_t sensor_hndl, int token,
> uint32_t *sensor_data);
>
> Index: b/arch/powerpc/platforms/powernv/opal-elog.c
> ===================================================================
> --- a/arch/powerpc/platforms/powernv/opal-elog.c
> +++ b/arch/powerpc/platforms/powernv/opal-elog.c
> @@ -243,7 +243,7 @@ static struct elog_obj *create_elog_obj(
>
> static void elog_work_fn(struct work_struct *work)
> {
> - size_t elog_size;
> + uint64_t elog_size;
> uint64_t log_id;
> uint64_t elog_type;
> int rc;
^ permalink raw reply
* Re: [PATCH v4] powernv: Dynamic Frequency Scaling Enablement
From: Viresh Kumar @ 2014-03-27 5:38 UTC (permalink / raw)
To: Gautham R. Shenoy
Cc: Linux PM list, linuxppc-dev@ozlabs.org, Anton Blanchard,
Srivatsa S. Bhat
In-Reply-To: <1395852947-22290-1-git-send-email-ego@linux.vnet.ibm.com>
On 26 March 2014 22:25, Gautham R. Shenoy <ego@linux.vnet.ibm.com> wrote:
> This is the v4 of the patchset to enable Dynamic Frequency Scaling
> on IBM PowerNV Platforms. I have incorporated the review comments
> from the previous version (can be found at [1]).
I wouldn't have added a cover-letter if I only have a single patch to
send. Would have rather used the not-to-be-commited field of the
patch instead.
^ permalink raw reply
* [PATCH 4/4] powerpc/powernv: Fix little endian issues OPAL error log code
From: Anton Blanchard @ 2014-03-27 5:20 UTC (permalink / raw)
To: stewart, benh, paulus; +Cc: linuxppc-dev
In-Reply-To: <20140327161849.791432d0@kryten>
Fix little endian issues with the OPAL error log code.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: b/arch/powerpc/platforms/powernv/opal-elog.c
===================================================================
--- a/arch/powerpc/platforms/powernv/opal-elog.c
+++ b/arch/powerpc/platforms/powernv/opal-elog.c
@@ -243,18 +243,25 @@ static struct elog_obj *create_elog_obj(
static void elog_work_fn(struct work_struct *work)
{
+ __be64 size;
+ __be64 id;
+ __be64 type;
uint64_t elog_size;
uint64_t log_id;
uint64_t elog_type;
int rc;
char name[2+16+1];
- rc = opal_get_elog_size(&log_id, &elog_size, &elog_type);
+ rc = opal_get_elog_size(&id, &size, &type);
if (rc != OPAL_SUCCESS) {
pr_err("ELOG: Opal log read failed\n");
return;
}
+ elog_size = be64_to_cpu(size);
+ log_id = be64_to_cpu(id);
+ elog_type = be64_to_cpu(type);
+
BUG_ON(elog_size > OPAL_MAX_ERRLOG_SIZE);
if (elog_size >= OPAL_MAX_ERRLOG_SIZE)
Index: b/arch/powerpc/include/asm/opal.h
===================================================================
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -851,7 +851,7 @@ int64_t opal_lpc_read(uint32_t chip_id,
uint32_t addr, __be32 *data, uint32_t sz);
int64_t opal_read_elog(uint64_t buffer, uint64_t size, uint64_t log_id);
-int64_t opal_get_elog_size(uint64_t *log_id, uint64_t *size, uint64_t *elog_type);
+int64_t opal_get_elog_size(__be64 *log_id, __be64 *size, __be64 *elog_type);
int64_t opal_write_elog(uint64_t buffer, uint64_t size, uint64_t offset);
int64_t opal_send_ack_elog(uint64_t log_id);
void opal_resend_pending_logs(void);
^ permalink raw reply
* [PATCH 3/4] powerpc/powernv: Fix little endian issues with opal_do_notifier calls
From: Anton Blanchard @ 2014-03-27 5:20 UTC (permalink / raw)
To: stewart, benh, paulus; +Cc: linuxppc-dev
In-Reply-To: <20140327161849.791432d0@kryten>
The bitmap in opal_poll_events and opal_handle_interrupt is
big endian, so we need to byteswap it on little endian builds.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: b/arch/powerpc/platforms/powernv/opal.c
===================================================================
--- a/arch/powerpc/platforms/powernv/opal.c
+++ b/arch/powerpc/platforms/powernv/opal.c
@@ -216,14 +216,14 @@ void opal_notifier_update_evt(uint64_t e
void opal_notifier_enable(void)
{
int64_t rc;
- uint64_t evt = 0;
+ __be64 evt = 0;
atomic_set(&opal_notifier_hold, 0);
/* Process pending events */
rc = opal_poll_events(&evt);
if (rc == OPAL_SUCCESS && evt)
- opal_do_notifier(evt);
+ opal_do_notifier(be64_to_cpu(evt));
}
void opal_notifier_disable(void)
@@ -501,7 +501,7 @@ static irqreturn_t opal_interrupt(int ir
opal_handle_interrupt(virq_to_hw(irq), &events);
- opal_do_notifier(events);
+ opal_do_notifier(be64_to_cpu(events));
return IRQ_HANDLED;
}
^ permalink raw reply
* [PATCH 2/4] powerpc/powernv: Remove some OPAL function declaration duplication
From: Anton Blanchard @ 2014-03-27 5:19 UTC (permalink / raw)
To: stewart, benh, paulus; +Cc: linuxppc-dev
In-Reply-To: <20140327161849.791432d0@kryten>
We had some duplication of the internal OPAL functions.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: b/arch/powerpc/include/asm/opal.h
===================================================================
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -877,7 +877,8 @@ int64_t opal_sensor_read(uint32_t sensor
uint32_t *sensor_data);
/* Internal functions */
-extern int early_init_dt_scan_opal(unsigned long node, const char *uname, int depth, void *data);
+extern int early_init_dt_scan_opal(unsigned long node, const char *uname,
+ int depth, void *data);
extern int early_init_dt_scan_recoverable_ranges(unsigned long node,
const char *uname, int depth, void *data);
@@ -886,10 +887,6 @@ extern int opal_put_chars(uint32_t vterm
extern void hvc_opal_init_early(void);
-/* Internal functions */
-extern int early_init_dt_scan_opal(unsigned long node, const char *uname,
- int depth, void *data);
-
extern int opal_notifier_register(struct notifier_block *nb);
extern int opal_message_notifier_register(enum OpalMessageType msg_type,
struct notifier_block *nb);
@@ -897,9 +894,6 @@ extern void opal_notifier_enable(void);
extern void opal_notifier_disable(void);
extern void opal_notifier_update_evt(uint64_t evt_mask, uint64_t evt_val);
-extern int opal_get_chars(uint32_t vtermno, char *buf, int count);
-extern int opal_put_chars(uint32_t vtermno, const char *buf, int total_len);
-
extern int __opal_async_get_token(void);
extern int opal_async_get_token_interruptible(void);
extern int __opal_async_release_token(int token);
@@ -907,8 +901,6 @@ extern int opal_async_release_token(int
extern int opal_async_wait_response(uint64_t token, struct opal_msg *msg);
extern int opal_get_sensor_data(u32 sensor_hndl, u32 *sensor_data);
-extern void hvc_opal_init_early(void);
-
struct rtc_time;
extern int opal_set_rtc_time(struct rtc_time *tm);
extern void opal_get_rtc_time(struct rtc_time *tm);
^ permalink raw reply
* [PATCH 1/4] powerpc/powernv: Use uint64_t instead of size_t in OPAL APIs
From: Anton Blanchard @ 2014-03-27 5:18 UTC (permalink / raw)
To: stewart, benh, paulus; +Cc: linuxppc-dev
Using size_t in our APIs is asking for trouble, especially
when some OPAL calls use size_t pointers.
Signed-off-by: Anton Blanchard <anton@samba.org>
---
Index: b/arch/powerpc/include/asm/opal.h
===================================================================
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -850,8 +850,8 @@ int64_t opal_lpc_write(uint32_t chip_id,
int64_t opal_lpc_read(uint32_t chip_id, enum OpalLPCAddressType addr_type,
uint32_t addr, __be32 *data, uint32_t sz);
-int64_t opal_read_elog(uint64_t buffer, size_t size, uint64_t log_id);
-int64_t opal_get_elog_size(uint64_t *log_id, size_t *size, uint64_t *elog_type);
+int64_t opal_read_elog(uint64_t buffer, uint64_t size, uint64_t log_id);
+int64_t opal_get_elog_size(uint64_t *log_id, uint64_t *size, uint64_t *elog_type);
int64_t opal_write_elog(uint64_t buffer, uint64_t size, uint64_t offset);
int64_t opal_send_ack_elog(uint64_t log_id);
void opal_resend_pending_logs(void);
@@ -866,13 +866,13 @@ int64_t opal_dump_read(uint32_t dump_id,
int64_t opal_dump_ack(uint32_t dump_id);
int64_t opal_dump_resend_notification(void);
-int64_t opal_get_msg(uint64_t buffer, size_t size);
-int64_t opal_check_completion(uint64_t buffer, size_t size, uint64_t token);
+int64_t opal_get_msg(uint64_t buffer, uint64_t size);
+int64_t opal_check_completion(uint64_t buffer, uint64_t size, uint64_t token);
int64_t opal_sync_host_reboot(void);
int64_t opal_get_param(uint64_t token, uint32_t param_id, uint64_t buffer,
- size_t length);
+ uint64_t length);
int64_t opal_set_param(uint64_t token, uint32_t param_id, uint64_t buffer,
- size_t length);
+ uint64_t length);
int64_t opal_sensor_read(uint32_t sensor_hndl, int token,
uint32_t *sensor_data);
Index: b/arch/powerpc/platforms/powernv/opal-elog.c
===================================================================
--- a/arch/powerpc/platforms/powernv/opal-elog.c
+++ b/arch/powerpc/platforms/powernv/opal-elog.c
@@ -243,7 +243,7 @@ static struct elog_obj *create_elog_obj(
static void elog_work_fn(struct work_struct *work)
{
- size_t elog_size;
+ uint64_t elog_size;
uint64_t log_id;
uint64_t elog_type;
int rc;
^ permalink raw reply
* RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
From: Li.Xiubo @ 2014-03-27 4:18 UTC (permalink / raw)
To: guangyu.chen@freescale.com
Cc: alsa-devel@alsa-project.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <20140327035726.GA16359@MrMyself>
> > So let's just ignore the clearance of these bits in isr().
> >
> > +++++
> > SAI Transmit Control Register (I2S1_TCSR) : 32 : R/W : 0000_0000h
>=20
> I'm talking about FWF and FRF bits, not TCSR as a register.
>=20
> > -----
> >
> > I have checked in the Vybrid and LS1 SoC datasheets, and they are all t=
he
> > Same as above, and nothing else.
> >
> > Have I missed ?
>=20
> What i.MX IC team told me is SAI ignores what we do to FWF and FRF, so yo=
u
> don't need to worry about it at all unless Vybrid makes them writable, in
> which case we may also need to clear these bits and confirm with Vybrid I=
C
> team if they're also W1C.
>=20
Well, if so, that's fine.
Thanks,
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
From: Nicolin Chen @ 2014-03-27 3:57 UTC (permalink / raw)
To: Xiubo Li-B47053
Cc: alsa-devel@alsa-project.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <62630844c7434df6937ca25d3f47b656@BY2PR03MB505.namprd03.prod.outlook.com>
On Thu, Mar 27, 2014 at 12:06:53PM +0800, Xiubo Li-B47053 wrote:
> > > > > > > > + if (xcsr & FSL_SAI_CSR_FWF)
> > > > > > > > + dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > > > > > +
> > > > > > > > + if (xcsr & FSL_SAI_CSR_FRF)
> > > > > > > > + dev_dbg(dev, "isr: Transmit FIFO watermark has been
> > > > reached\n");
> > > > > > > > +
> > > > > > >
> > > > > > > While are these ones really needed to clear manually ?
> > > > > >
> > > > > > The reference manual doesn't mention about the requirement. So SAI
> > should
> > > > do
> > > > > > the self-clearance.
> > > > >
> > > > > Yes, I do think we should let it do the self-clearance, and shouldn't
> > > > interfere
> > > > > of them...
> > > >
> > > > SAI is supposed to ignore the interference, isn't it?
> > > >
> > >
> > > Maybe, but I'm not very sure.
> > > And these bits are all writable and readable.
> >
> > Double-confirmed? Because FWF and FRF should be read-only bits.
> >
>
> So let's just ignore the clearance of these bits in isr().
>
> +++++
> SAI Transmit Control Register (I2S1_TCSR) : 32 : R/W : 0000_0000h
I'm talking about FWF and FRF bits, not TCSR as a register.
> -----
>
> I have checked in the Vybrid and LS1 SoC datasheets, and they are all the
> Same as above, and nothing else.
>
> Have I missed ?
What i.MX IC team told me is SAI ignores what we do to FWF and FRF, so you
don't need to worry about it at all unless Vybrid makes them writable, in
which case we may also need to clear these bits and confirm with Vybrid IC
team if they're also W1C.
^ permalink raw reply
* RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
From: Li.Xiubo @ 2014-03-27 4:06 UTC (permalink / raw)
To: guangyu.chen@freescale.com
Cc: alsa-devel@alsa-project.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <20140327033117.GD16197@MrMyself>
> > > > > > > + if (xcsr & FSL_SAI_CSR_FWF)
> > > > > > > + dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > > > > +
> > > > > > > + if (xcsr & FSL_SAI_CSR_FRF)
> > > > > > > + dev_dbg(dev, "isr: Transmit FIFO watermark has been
> > > reached\n");
> > > > > > > +
> > > > > >
> > > > > > While are these ones really needed to clear manually ?
> > > > >
> > > > > The reference manual doesn't mention about the requirement. So SA=
I
> should
> > > do
> > > > > the self-clearance.
> > > >
> > > > Yes, I do think we should let it do the self-clearance, and shouldn=
't
> > > interfere
> > > > of them...
> > >
> > > SAI is supposed to ignore the interference, isn't it?
> > >
> >
> > Maybe, but I'm not very sure.
> > And these bits are all writable and readable.
>=20
> Double-confirmed? Because FWF and FRF should be read-only bits.
>=20
So let's just ignore the clearance of these bits in isr().
+++++
SAI Transmit Control Register (I2S1_TCSR) : 32 : R/W : 0000_0000h
-----
I have checked in the Vybrid and LS1 SoC datasheets, and they are all the
Same as above, and nothing else.
Have I missed ?
^ permalink raw reply
* RE: [PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module
From: Jason.Jin @ 2014-03-27 3:57 UTC (permalink / raw)
To: Timur Tabi
Cc: Scott Wood, linux-fbdev@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, Dongsheng.Wang@freescale.com
In-Reply-To: <53339F03.7010004@tabi.org>
> Jason.Jin@freescale.com wrote:
> > [Jason Jin-R64188] It's not hackish, we can provide the pixel clock
> register in the DIU node, I did not provide the dts update as this is
> only tested on T1040 platform. For other platforms such as p1022 and 8610=
,
> we still can use the pixel clock setting function in the platform.
> >
> > The dts node update for T1040 is:
> > display:display@180000 {
> > compatible =3D "fsl,t1040-diu", "fsl,diu";
> > - reg =3D <0x180000 1000>;
> > + reg =3D <0x180000 1000 0xfc028 4>;
> > interrupts =3D <74 2 0 0>;
> > };
>=20
> This is hackish because you're specifying a single register that you want
> to preserve in the DTS file, instead of a platform function which is
> where it's supposed to be.
>=20
[Jason Jin-R64188] The pixel clock register is actually part of the DIU reg=
isters although it is not implemented in the diu module. Actually we can us=
e it to set pixel clock in the driver not just saving it in suspend. We can=
provide the patch for discussion.=20
> I will think about this some more. I think you are trying too hard to
> avoid a platform file, which is why some of this code is hackish to me.
[Jason Jin-R64188] Thanks. Could you please share you thinking for how to s=
etup the platform file then?
^ permalink raw reply
* Re: [PATCH v4] powernv, cpufreq: cpufreq driver for powernv platform
From: Anton Blanchard @ 2014-03-27 3:56 UTC (permalink / raw)
To: Gautham R. Shenoy
Cc: Linux PM list, Viresh Kumar, linuxppc-dev, srivatsa.bhat
In-Reply-To: <1395852947-22290-2-git-send-email-ego@linux.vnet.ibm.com>
Hi Gautham,
> Backend driver to dynamically set voltage and frequency on
> IBM POWER non-virtualized platforms. Power management SPRs
> are used to set the required PState.
I tested this version on ppc64le, it still looks good. I'm already
a Signed-off-by on the patch, but feel free to add:
Tested-by: Anton Blanchard <anton@samba.org>
Anton
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
From: Nicolin Chen @ 2014-03-27 3:31 UTC (permalink / raw)
To: Xiubo Li-B47053
Cc: alsa-devel@alsa-project.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <12f887b3889a41849026965f0cf7db1f@BY2PR03MB505.namprd03.prod.outlook.com>
On Thu, Mar 27, 2014 at 11:41:02AM +0800, Xiubo Li-B47053 wrote:
>
> > Subject: Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
> >
> > On Thu, Mar 27, 2014 at 10:53:50AM +0800, Xiubo Li-B47053 wrote:
> > > > On Thu, Mar 27, 2014 at 10:13:48AM +0800, Xiubo Li-B47053 wrote:
> > > > > > + regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > > > > + regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> > > > > > +
> > > > > > + if (xcsr & FSL_SAI_CSR_WSF)
> > > > > > + dev_dbg(dev, "isr: Start of Tx word detected\n");
> > > > > > +
> > > > > > + if (xcsr & FSL_SAI_CSR_SEF)
> > > > > > + dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> > > > > > +
> > > > > > + if (xcsr & FSL_SAI_CSR_FEF)
> > > > > > + dev_dbg(dev, "isr: Transmit underrun detected\n");
> > > > > > +
> > > > >
> > > > > Actually, the above three isrs should to write a logic 1 to this field
> > > > > to clear this flag.
> > > > >
> > > > >
> > > > > > + if (xcsr & FSL_SAI_CSR_FWF)
> > > > > > + dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > > > +
> > > > > > + if (xcsr & FSL_SAI_CSR_FRF)
> > > > > > + dev_dbg(dev, "isr: Transmit FIFO watermark has been
> > reached\n");
> > > > > > +
> > > > >
> > > > > While are these ones really needed to clear manually ?
> > > >
> > > > The reference manual doesn't mention about the requirement. So SAI should
> > do
> > > > the self-clearance.
> > >
> > > Yes, I do think we should let it do the self-clearance, and shouldn't
> > interfere
> > > of them...
> >
> > SAI is supposed to ignore the interference, isn't it?
> >
>
> Maybe, but I'm not very sure.
> And these bits are all writable and readable.
Double-confirmed? Because FWF and FRF should be read-only bits.
^ permalink raw reply
* Re: [PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module
From: Timur Tabi @ 2014-03-27 3:46 UTC (permalink / raw)
To: Jason.Jin@freescale.com
Cc: Scott Wood, linux-fbdev@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, Dongsheng.Wang@freescale.com
In-Reply-To: <e3e45edb388c482eb53474e18575f227@BLUPR03MB373.namprd03.prod.outlook.com>
Jason.Jin@freescale.com wrote:
> [Jason Jin-R64188] It's not hackish, we can provide the pixel clock register in the DIU node, I did not provide the dts update as this is only tested on T1040 platform. For other platforms such as p1022 and 8610, we still can use the pixel clock setting function in the platform.
>
> The dts node update for T1040 is:
> display:display@180000 {
> compatible = "fsl,t1040-diu", "fsl,diu";
> - reg = <0x180000 1000>;
> + reg = <0x180000 1000 0xfc028 4>;
> interrupts = <74 2 0 0>;
> };
This is hackish because you're specifying a single register that you
want to preserve in the DTS file, instead of a platform function which
is where it's supposed to be.
I will think about this some more. I think you are trying too hard to
avoid a platform file, which is why some of this code is hackish to me.
^ permalink raw reply
* RE: [PATCH 1/2] video/fsl: make the diu driver work without platform hooks
From: Jason.Jin @ 2014-03-27 3:30 UTC (permalink / raw)
To: Timur Tabi
Cc: Scott Wood, linux-fbdev@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <533320A0.8010507@tabi.org>
> On 03/26/2014 12:41 PM, Jason Jin wrote:
> > This board sepecific initialization mechanism is not feasible i for
> > corenet platform as the corenet platform file is a abstraction of
> > serveral platforms.
>=20
> You can't make it 100% abstract. The DIU driver requires some sort of
> board support. I think you can make a generic platform file for the DIU.
>=20
[Jason Jin-R64188] Provide the generic diu platform file is reasonable, but=
it is will be just the board specific initialization file collection, if w=
e really need to reinitialize something in kernel.=20
> > However, the DIU is already initialized in u-boot and we can rely on
> > the settings in u-boot for corenet platform,
>=20
> I don't like that at all.
[Jason Jin-R64188] What's the potential issue of this? Most of the IPs need=
the platform initialization in u-boot. For example, the hwconfig in u-boot=
used for board specific settings for most platform.
The diu_ops structure is still open for any board specific initialization i=
f the platform needed. We at least can let the platform to select to use th=
e diu_ops or not. This is the purse for this patch.
>=20
> > the only
> > issue is that when DIU wake up from the deepsleep, some of the board
> > specific initialization will lost, such as the pixel clock setting.
>=20
> That is a BIG issue. This is why we have a platform file -- to handle
> the platform issues.
[Jason Jin-R64188] Yes, this is an issue, Actually, this is a SOC level iss=
ue, We can try to fix it in the driver by saving the pixel clock in suspend=
function and restored it in resume function.
Thanks.
^ permalink raw reply
* RE: [PATCH 2/2] video/fsl: Fix the sleep function for FSL DIU module
From: Jason.Jin @ 2014-03-27 3:42 UTC (permalink / raw)
To: Timur Tabi
Cc: Scott Wood, linux-fbdev@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, Dongsheng.Wang@freescale.com
In-Reply-To: <53332168.5050805@tabi.org>
>=20
> On 03/26/2014 12:41 PM, Jason Jin wrote:
> > + if (!diu_ops.set_pixel_clock) {
> > + data->saved_pixel_clock =3D 0;
> > + if (of_address_to_resource(ofdev->dev.of_node, 1, &res))
> > + pr_err(KERN_ERR "No pixel clock set func and no pixel
> node!\n");
> > + else {
> > + data->pixelclk_ptr =3D
> > + devm_ioremap(&ofdev->dev, res.start,
> resource_size(&res));
> > + if (!data->pixelclk_ptr) {
> > + pr_err(KERN_ERR "fslfb: could not map pixelclk
> register!\n");
> > + ret =3D -ENOMEM;
> > + } else
> > + data->saved_pixel_clock =3D in_be32(data-
> >pixelclk_ptr);
> > + }
> > + }
>=20
> This seems very hackish. What node does ofdev point to? I wonder if
> this code should be in the platform file instead.
>=20
[Jason Jin-R64188] It's not hackish, we can provide the pixel clock registe=
r in the DIU node, I did not provide the dts update as this is only tested =
on T1040 platform. For other platforms such as p1022 and 8610, we still can=
use the pixel clock setting function in the platform.=20
The dts node update for T1040 is:
display:display@180000 {
compatible =3D "fsl,t1040-diu", "fsl,diu";
- reg =3D <0x180000 1000>;
+ reg =3D <0x180000 1000 0xfc028 4>;
interrupts =3D <74 2 0 0>;
};
If saving the pixel clock register likes a hackish. We can provide more inf=
ormation in the node to move the pixel clock settings to the driver. It see=
ms that we only need to provide another parameter(the ratio) for pixel cloc=
k setting.
> Also, use dev_info() instead of pr_err, and never use exclamation marks
> in driver messages.
Ok, Thanks.
^ permalink raw reply
* RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
From: Li.Xiubo @ 2014-03-27 3:41 UTC (permalink / raw)
To: guangyu.chen@freescale.com
Cc: alsa-devel@alsa-project.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <20140327025647.GC16197@MrMyself>
> Subject: Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
>=20
> On Thu, Mar 27, 2014 at 10:53:50AM +0800, Xiubo Li-B47053 wrote:
> > > On Thu, Mar 27, 2014 at 10:13:48AM +0800, Xiubo Li-B47053 wrote:
> > > > > + regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > > > + regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> > > > > +
> > > > > + if (xcsr & FSL_SAI_CSR_WSF)
> > > > > + dev_dbg(dev, "isr: Start of Tx word detected\n");
> > > > > +
> > > > > + if (xcsr & FSL_SAI_CSR_SEF)
> > > > > + dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> > > > > +
> > > > > + if (xcsr & FSL_SAI_CSR_FEF)
> > > > > + dev_dbg(dev, "isr: Transmit underrun detected\n");
> > > > > +
> > > >
> > > > Actually, the above three isrs should to write a logic 1 to this fi=
eld
> > > > to clear this flag.
> > > >
> > > >
> > > > > + if (xcsr & FSL_SAI_CSR_FWF)
> > > > > + dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > > +
> > > > > + if (xcsr & FSL_SAI_CSR_FRF)
> > > > > + dev_dbg(dev, "isr: Transmit FIFO watermark has been
> reached\n");
> > > > > +
> > > >
> > > > While are these ones really needed to clear manually ?
> > >
> > > The reference manual doesn't mention about the requirement. So SAI sh=
ould
> do
> > > the self-clearance.
> >
> > Yes, I do think we should let it do the self-clearance, and shouldn't
> interfere
> > of them...
>=20
> SAI is supposed to ignore the interference, isn't it?
>=20
Maybe, but I'm not very sure.
And these bits are all writable and readable.
^ permalink raw reply
* RE: [PATCH 1/2] video/fsl: make the diu driver work without platform hooks
From: Jason.Jin @ 2014-03-27 3:38 UTC (permalink / raw)
To: Timur Tabi
Cc: Scott Wood, linux-fbdev@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <53339C96.3070406@tabi.org>
> Jason.Jin@freescale.com wrote:
> >>>>> However, the DIU is already initialized in u-boot and we can rely
> >>>>> on the settings in u-boot for corenet platform,
> >>>
> >>> I don't like that at all.
> > [Jason Jin-R64188] What's the potential issue of this? Most of the IPs
> > need the platform initialization in u-boot. For example, the hwconfig
> > in u-boot used for board specific settings for most platform.
>=20
> The potential problem is that it's not stable. Power management breaks
> your code.
>=20
> I won't NACK your patch, but it feels like a band-aid rather than a real
> solution.
>=20
[Jason Jin-R64188] Thanks. The power management will break the pixel clock =
only. We can try to fix it separately. I'll try to explain it in that mail.
^ permalink raw reply
* Re: [PATCH 1/2] video/fsl: make the diu driver work without platform hooks
From: Timur Tabi @ 2014-03-27 3:35 UTC (permalink / raw)
To: Jason.Jin@freescale.com
Cc: Scott Wood, linux-fbdev@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <f6883c3f759a4544936acec5dc7e542b@BLUPR03MB373.namprd03.prod.outlook.com>
Jason.Jin@freescale.com wrote:
>>>>> However, the DIU is already initialized in u-boot and we can
>>>>> rely on the settings in u-boot for corenet platform,
>>>
>>> I don't like that at all.
> [Jason Jin-R64188] What's the potential issue of this? Most of the
> IPs need the platform initialization in u-boot. For example, the
> hwconfig in u-boot used for board specific settings for most
> platform.
The potential problem is that it's not stable. Power management breaks
your code.
I won't NACK your patch, but it feels like a band-aid rather than a real
solution.
^ permalink raw reply
* Re: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
From: Nicolin Chen @ 2014-03-27 2:56 UTC (permalink / raw)
To: Xiubo Li-B47053
Cc: alsa-devel@alsa-project.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <1e2d210764724c7e834c5307b54c37ab@BY2PR03MB505.namprd03.prod.outlook.com>
On Thu, Mar 27, 2014 at 10:53:50AM +0800, Xiubo Li-B47053 wrote:
> > On Thu, Mar 27, 2014 at 10:13:48AM +0800, Xiubo Li-B47053 wrote:
> > > > + regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > > + regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> > > > +
> > > > + if (xcsr & FSL_SAI_CSR_WSF)
> > > > + dev_dbg(dev, "isr: Start of Tx word detected\n");
> > > > +
> > > > + if (xcsr & FSL_SAI_CSR_SEF)
> > > > + dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> > > > +
> > > > + if (xcsr & FSL_SAI_CSR_FEF)
> > > > + dev_dbg(dev, "isr: Transmit underrun detected\n");
> > > > +
> > >
> > > Actually, the above three isrs should to write a logic 1 to this field
> > > to clear this flag.
> > >
> > >
> > > > + if (xcsr & FSL_SAI_CSR_FWF)
> > > > + dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > > +
> > > > + if (xcsr & FSL_SAI_CSR_FRF)
> > > > + dev_dbg(dev, "isr: Transmit FIFO watermark has been reached\n");
> > > > +
> > >
> > > While are these ones really needed to clear manually ?
> >
> > The reference manual doesn't mention about the requirement. So SAI should do
> > the self-clearance.
>
> Yes, I do think we should let it do the self-clearance, and shouldn't interfere
> of them...
SAI is supposed to ignore the interference, isn't it?
^ permalink raw reply
* RE: [PATCH] ASoC: fsl_sai: Add isr to deal with error flag
From: Li.Xiubo @ 2014-03-27 2:53 UTC (permalink / raw)
To: guangyu.chen@freescale.com
Cc: alsa-devel@alsa-project.org, broonie@kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <20140327023617.GB16197@MrMyself>
> On Thu, Mar 27, 2014 at 10:13:48AM +0800, Xiubo Li-B47053 wrote:
> > > + regmap_read(sai->regmap, FSL_SAI_TCSR, &xcsr);
> > > + regmap_write(sai->regmap, FSL_SAI_TCSR, xcsr);
> > > +
> > > + if (xcsr & FSL_SAI_CSR_WSF)
> > > + dev_dbg(dev, "isr: Start of Tx word detected\n");
> > > +
> > > + if (xcsr & FSL_SAI_CSR_SEF)
> > > + dev_dbg(dev, "isr: Tx Frame sync error detected\n");
> > > +
> > > + if (xcsr & FSL_SAI_CSR_FEF)
> > > + dev_dbg(dev, "isr: Transmit underrun detected\n");
> > > +
> >
> > Actually, the above three isrs should to write a logic 1 to this field
> > to clear this flag.
> >
> >
> > > + if (xcsr & FSL_SAI_CSR_FWF)
> > > + dev_dbg(dev, "isr: Enabled transmit FIFO is empty\n");
> > > +
> > > + if (xcsr & FSL_SAI_CSR_FRF)
> > > + dev_dbg(dev, "isr: Transmit FIFO watermark has been reached\n");
> > > +
> >
> > While are these ones really needed to clear manually ?
>=20
> The reference manual doesn't mention about the requirement. So SAI should=
do
> the self-clearance.
Yes, I do think we should let it do the self-clearance, and shouldn't inter=
fere
of them...
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox