* [PATCH 3/3] cpufreq: pmac64: enable cpufreq on iMac G5 (iSight) model
From: Aaro Koskinen @ 2013-07-23 20:24 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Benjamin Herrenschmidt,
Nick Piggin, linux-pm, linuxppc-dev
Cc: Aaro Koskinen
In-Reply-To: <1374611079-28091-1-git-send-email-aaro.koskinen@iki.fi>
Enable cpufreq on iMac G5 (iSight) model. Tested with the 2.1 GHz version.
Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
drivers/cpufreq/pmac64-cpufreq.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/cpufreq/pmac64-cpufreq.c b/drivers/cpufreq/pmac64-cpufreq.c
index f9e399b..1f352e5 100644
--- a/drivers/cpufreq/pmac64-cpufreq.c
+++ b/drivers/cpufreq/pmac64-cpufreq.c
@@ -399,7 +399,8 @@ static int __init g5_neo2_cpufreq_init(struct device_node *cpus)
/* Check supported platforms */
if (of_machine_is_compatible("PowerMac8,1") ||
of_machine_is_compatible("PowerMac8,2") ||
- of_machine_is_compatible("PowerMac9,1"))
+ of_machine_is_compatible("PowerMac9,1") ||
+ of_machine_is_compatible("PowerMac12,1"))
use_volts_smu = 1;
else if (of_machine_is_compatible("PowerMac11,2"))
use_volts_vdnap = 1;
--
1.8.3.2
^ permalink raw reply related
* [PATCH 2/3] cpufreq: pmac64: re-estimate G5 cpufreq transition latency
From: Aaro Koskinen @ 2013-07-23 20:24 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Benjamin Herrenschmidt,
Nick Piggin, linux-pm, linuxppc-dev
Cc: Aaro Koskinen
In-Reply-To: <1374611079-28091-1-git-send-email-aaro.koskinen@iki.fi>
The latency is in milliseconds scale rather than microseconds based on
measurements on iMac G5 and Xscale G5. The patch also enables to use
ondemand governor on the latter.
Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
drivers/cpufreq/pmac64-cpufreq.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/pmac64-cpufreq.c b/drivers/cpufreq/pmac64-cpufreq.c
index 674807d..f9e399b 100644
--- a/drivers/cpufreq/pmac64-cpufreq.c
+++ b/drivers/cpufreq/pmac64-cpufreq.c
@@ -85,7 +85,8 @@ static int (*g5_query_freq)(void);
static DEFINE_MUTEX(g5_switch_mutex);
-static unsigned long transition_latency;
+/* A conservative estimate, based on Xserve G5 and iMac G5 (iSight). */
+static const unsigned long transition_latency = 10 * NSEC_PER_MSEC;
#ifdef CONFIG_PMAC_SMU
@@ -499,7 +500,6 @@ static int __init g5_neo2_cpufreq_init(struct device_node *cpus)
g5_cpu_freqs[1].frequency = max_freq/2;
/* Set callbacks */
- transition_latency = 12000;
g5_switch_freq = g5_scom_switch_freq;
g5_query_freq = g5_scom_query_freq;
freq_method = "SCOM";
@@ -675,7 +675,6 @@ static int __init g5_pm72_cpufreq_init(struct device_node *cpus)
g5_cpu_freqs[1].frequency = min_freq;
/* Set callbacks */
- transition_latency = CPUFREQ_ETERNAL;
g5_switch_volt = g5_pfunc_switch_volt;
g5_switch_freq = g5_pfunc_switch_freq;
g5_query_freq = g5_pfunc_query_freq;
--
1.8.3.2
^ permalink raw reply related
* [PATCH 1/3] cpufreq: pmac64: speed up frequency switch
From: Aaro Koskinen @ 2013-07-23 20:24 UTC (permalink / raw)
To: Rafael J. Wysocki, Viresh Kumar, Benjamin Herrenschmidt,
Nick Piggin, linux-pm, linuxppc-dev
Cc: Aaro Koskinen
Some functions on switch path use msleep() which is inaccurate, and
depends on HZ. With HZ=100 msleep(1) takes actually over ten times longer.
Using usleep_range() we get more accurate sleeps.
I measured the "pfunc_slewing_done" polling to take 300us at max (on
2.3GHz dual-processor Xserve G5), so using 500us sleep there should
be fine.
With the patch, g5_switch_freq() duration drops from ~50ms to ~10ms on
Xserve with HZ=100.
Signed-off-by: Aaro Koskinen <aaro.koskinen@iki.fi>
---
drivers/cpufreq/pmac64-cpufreq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/cpufreq/pmac64-cpufreq.c b/drivers/cpufreq/pmac64-cpufreq.c
index 7ba4234..674807d 100644
--- a/drivers/cpufreq/pmac64-cpufreq.c
+++ b/drivers/cpufreq/pmac64-cpufreq.c
@@ -141,7 +141,7 @@ static void g5_vdnap_switch_volt(int speed_mode)
pmf_call_one(pfunc_vdnap0_complete, &args);
if (done)
break;
- msleep(1);
+ usleep_range(1000, 1000);
}
if (done == 0)
printk(KERN_WARNING "cpufreq: Timeout in clock slewing !\n");
@@ -240,7 +240,7 @@ static void g5_pfunc_switch_volt(int speed_mode)
if (pfunc_cpu1_volt_low)
pmf_call_one(pfunc_cpu1_volt_low, NULL);
}
- msleep(10); /* should be faster , to fix */
+ usleep_range(10000, 10000); /* should be faster , to fix */
}
/*
@@ -285,7 +285,7 @@ static int g5_pfunc_switch_freq(int speed_mode)
pmf_call_one(pfunc_slewing_done, &args);
if (done)
break;
- msleep(1);
+ usleep_range(500, 500);
}
if (done == 0)
printk(KERN_WARNING "cpufreq: Timeout in clock slewing !\n");
--
1.8.3.2
^ permalink raw reply related
* Re: [RFC] power/mpc85xx: Add delay after enabling I2C master
From: Scott Wood @ 2013-07-23 18:43 UTC (permalink / raw)
To: York Sun; +Cc: albrecht.dress, linuxppc-dev
In-Reply-To: <51EEA34A.9050400@freescale.com>
On 07/23/2013 10:37:46 AM, York Sun wrote:
> On 07/22/2013 05:33 PM, Scott Wood wrote:
> > On Mon, May 13, 2013 at 02:27:08PM -0700, York Sun wrote:
> >> Erratum A-006037 indicates I2C controller executes the write to =20
> I2CCR only
> >> after it sees SCL idle for 64K cycle of internal I2C controller =20
> clocks. If
> >> during this waiting period, I2C controller is disabled (I2CCR[MEN] =20
> set to
> >> 0), then the controller could end in bad state, and hang the =20
> future access
> >> to I2C register.
> >>
> >> The mpc_i2c_fixup() function tries to recover the bus from a =20
> stalled state
> >> where the 9th clock pulse wasn't generated. However, this =20
> workaround
> >> disables and enables I2C controller without meeting waiting =20
> requirement of
> >> this erratum.
> >>
> >> This erratum applies to some 85xx SoCs. It is safe to apply to all =20
> of them
> >> for mpc_i2c_fixup().
> >>
> >> Signed-off-by: York Sun <yorksun@freescale.com>
> >>
> >> ---
> >> I'd like to get rid of the #ifdef if mpc5121 is OK with the longer =20
> delay.
> >
> > Are mpc5121 and mpc85xx the only things that use this?
>=20
> No. 83xx and 86xx also uses this file. But I am only unsure if mpc52xx
> is OK with this extended delay. I guess they are but I don't have a
> proof, or someone to confirm.
>=20
> >
> > Are you sure the delay always works out to be longer? What is the
> > relationship between fsl_get_sys_freq() and i2c->real_clk?
>=20
> Yes. The max divider from sys clock to i2c clcok is 32K.
> i2c->real_clk is the clock I2C controller pumps out, not its internal =20
> operation clock.
32K is the max for all implementations?
BTW, Where does the "2000000" come from? Shouldn't it be 1000000 if =20
you're converting to usec? If you're trying to add some slack, say so =20
rather than having a comment suggest that the output of that formula is =20
64K cycles. Or is there an implicit assumption that i2c runs at half =20
the system frequency? Is that assumption true for all implementations =20
that have this erratum?
> > In any case, you should send this patch to the i2c maintainer and =20
> list.
> >
>=20
> I don't have the name on top of my head. Is that =20
> linux-i2c@vger.kernel.org?
Yes, and Wolfram Sang <wsa@the-dreams.de> is the maintainer. This is =20
listed in the MAINTAINERS file.
-Scott=
^ permalink raw reply
* Re: cpuidle/pseries: Fix kernel command line parameter smt-snooze-delay
From: Deepthi Dharwar @ 2013-07-23 16:21 UTC (permalink / raw)
To: Michael Ellerman
Cc: Preeti Murthy, PowerPC email list, linux-kernel@vger.kernel.org,
Srivatsa S. Bhat
In-Reply-To: <20130723140256.GH31944@concordia>
On 07/23/2013 07:32 PM, Michael Ellerman wrote:
> On Tue, Jul 23, 2013 at 10:23:57AM +0530, Deepthi Dharwar wrote:
>> smt-snooze-delay is a tun-able provided currently on powerpc to delay the
>> entry of an idle cpu to NAP state. By default, the value is 100us,
>> which is entry criteria for NAP state i.e only if the idle period is
>> above 100us it would enter NAP. Value of -1 disables entry into NAP.
>> This value can be set either through sysfs, ppc64_cpu util or by
>> passing it via kernel command line. Currently this feature is broken
>> when the value is passed via the kernel command line.
>
> Has it always been broken? Or just since some commit?
Yes, it is broken since inclusion of pseries back-end driver, around 3.3
kernel time-frame.
>> This patch aims to fix this, by taking the appropriate action
>> based on the value after the pseries driver is registered.
>> This check is carried on in the back-end driver rather than
>> setup_smt_snooze_delay(), as one is not sure if the cpuidle driver
>> is even registered when setup routine is executed.
>> Also, this fixes re-enabling of NAP states by setting appropriate
>> value without having to reboot using smt-snooze-delay parameter.
>>
>> Also, to note is, smt-snooze-delay is per-cpu variable.
>> This can be used to enable/disable NAP on per-cpu
>> basis using sysfs but when this variable is passed
>> via kernel command line or using the smt-snooze-delay
>> it applies to all the cpus. Per-cpu tuning can
>> only be done via sysfs.
>>
>> Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
>> ---
>> arch/powerpc/platforms/pseries/processor_idle.c | 34 ++++++++++++++++++-----
>> 1 file changed, 27 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
>> index 4644efa0..8133f50 100644
>> --- a/arch/powerpc/platforms/pseries/processor_idle.c
>> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
>> @@ -170,18 +170,36 @@ static struct cpuidle_state shared_states[MAX_IDLE_STATE_COUNT] = {
>> void update_smt_snooze_delay(int cpu, int residency)
>> {
>> struct cpuidle_driver *drv = cpuidle_get_driver();
>> - struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
>> + struct cpuidle_device *dev;
>>
>> if (cpuidle_state_table != dedicated_states)
>> return;
>>
>> - if (residency < 0) {
>> - /* Disable the Nap state on that cpu */
>> - if (dev)
>> - dev->states_usage[1].disable = 1;
>> - } else
>> - if (drv)
>> + if (!drv)
>> + return;
>> +
>> + if (cpu == -1) {
>> + if (residency < 0) {
>> + /* Disable NAP on all cpus */
>> + drv->states[1].disabled = true;
>> + } else {
>> drv->states[1].target_residency = residency;
>> + drv->states[1].disabled = false;
>> + }
>> + return;
>> + }
>> +
>> + dev = per_cpu(cpuidle_devices, cpu);
>> + if (!dev)
>> + return;
>> +
>> + if (residency < 0)
>> + dev->states_usage[1].disable = 1;
>> + else {
>> + drv->states[1].target_residency = residency;
>> + drv->states[1].disabled = false;
>> + dev->states_usage[1].disable = 0;
>
> Why are we indexing into all these array with '1' ?
We are disabling/enabling NAP state, which indexes into pseries cpuidle
state table array with index value of 1.
smt-snooze-delay of -1, disables NAP state.
Else, we need to set the residency of NAP state i.e minimum time CPU
should spend in NAP state ( based on which menu governor takes a
decision on selection of Idle state) there by delaying the entry to NAP
state.
Thanks for the review.
Regards,
Deepthi
> cheers
>
^ permalink raw reply
* Re: [3/4] powerpc/85xx: Add C293PCIE board support
From: Scott Wood @ 2013-07-23 16:22 UTC (permalink / raw)
To: Liu Po-B43644
Cc: Wood Scott-B07421, Hu Mingkai-B21284, linuxppc-dev@ozlabs.org
In-Reply-To: <D473A0D087F4EA47A30C37E4637E25E609DAD260@039-SN2MPN1-021.039d.mgd.msft.net>
On 07/23/2013 02:47:18 AM, Liu Po-B43644 wrote:
> > > + partition@1900000 {
> > > + /* 7MB for User Area */
> > > + reg =3D <0x01900000 0x00700000>;
> > > + label =3D "NAND User area";
> > > + };
> >
> > Above you say there's 4 GiB of NAND, but here you define =20
> partitions that
> > only cover 32 MiB.
> Can I set one partion include all other space(4GB- 32MB) with label =20
> name "Others"?
Are you sure you don't want to leave more room for the RFS? And what =20
is the difference between "user area" and "others"?
> > > diff --git a/arch/powerpc/platforms/85xx/c293pcie.c
> > > b/arch/powerpc/platforms/85xx/c293pcie.c
> > > new file mode 100644
> > > index 0000000..75dda12
> > > --- /dev/null
> > > +++ b/arch/powerpc/platforms/85xx/c293pcie.c
> > > @@ -0,0 +1,82 @@
> > > +/*
> > > + * C293PCIE Board Setup
> > > + *
> > > + * Copyright 2013 Freescale Semiconductor Inc.
> > > + *
> > > + * This program is free software; you can redistribute it =20
> and/or
> > > +modify it
> > > + * under the terms of the GNU General Public License as =20
> published
> > > +by the
> > > + * Free Software Foundation; either version 2 of the License, =20
> or
> > > +(at your
> > > + * option) any later version.
> > > + */
> > > +
> > > +#include <linux/stddef.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/pci.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/of_platform.h>
> > > +
> > > +#include <asm/time.h>
> > > +#include <asm/machdep.h>
> > > +#include <asm/pci-bridge.h>
> > > +#include <mm/mmu_decl.h>
> > > +#include <asm/prom.h>
> > > +#include <asm/udbg.h>
> > > +#include <asm/mpic.h>
> > > +
> > > +#include <sysdev/fsl_soc.h>
> > > +#include <sysdev/fsl_pci.h>
> > > +
> > > +#include "mpc85xx.h"
> >
> > Are you sure you need all of these? I don't see any delays, for =20
> example.
> Thanks, I'll test and remove redundant includes.
Don't base it purely on testing -- you don't want to rely on =20
accidentally picking up a needed include from some other include (which =20
could change down the road). Base it on whether this file uses =20
something declared by the header in question.
-Scott=
^ permalink raw reply
* Re: [PATCH 1/2] Powerpc: Add voltage support in dts file
From: Scott Wood @ 2013-07-23 16:19 UTC (permalink / raw)
To: Haijun Zhang
Cc: linux-mmc, r39252, AFLEMING, cbouatmailru, cjb, linuxppc-dev,
Haijun Zhang
In-Reply-To: <1374548345-15017-1-git-send-email-Haijun.Zhang@freescale.com>
On 07/22/2013 09:59:04 PM, Haijun Zhang wrote:
> eSDHC of T4240 had 1.8v voltage support. Add this node to specify
> eSDHC voltage capacity. If this node not specified eSDHC driver
> still can read from eSDHC host capacity register.
>=20
> Signed-off-by: Haijun Zhang <haijun.zhang@freescale.com>
> Signed-off-by: Anton Vorontsov <cbouatmailru@gmail.com>
> ---
> changes for this patch set:
> - split from patch [PATCH 1/2] Powerpc: Add
> - voltage ranges support for T4
>=20
> Documentation/devicetree/bindings/mmc/fsl-esdhc.txt | 3 +++
> arch/powerpc/boot/dts/fsl/t4240si-post.dtsi | 1 +
> 2 files changed, 4 insertions(+)
>=20
> diff --git a/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt =20
> b/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
> index bd9be0b..f1ac253 100644
> --- a/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
> +++ b/Documentation/devicetree/bindings/mmc/fsl-esdhc.txt
> @@ -19,6 +19,8 @@ Optional properties:
> "bus-width =3D <1>" property.
> - sdhci,auto-cmd12: specifies that a controller can only handle =20
> auto
> CMD12.
> + - 3300 3300: specifies that eSDHC controller can support voltages =20
> ranges
> + from 3300 to 3300.
"3300 3300" is not a property name -- and the voltage-ranges property =20
should be defined more generically, as is done for mmc-spi-slot.txt.
-Scott=
^ permalink raw reply
* Re: [RFC] power/mpc85xx: Add delay after enabling I2C master
From: York Sun @ 2013-07-23 15:37 UTC (permalink / raw)
To: Scott Wood; +Cc: albrecht.dress, linuxppc-dev
In-Reply-To: <20130723003330.GA14649@home.buserror.net>
On 07/22/2013 05:33 PM, Scott Wood wrote:
> On Mon, May 13, 2013 at 02:27:08PM -0700, York Sun wrote:
>> Erratum A-006037 indicates I2C controller executes the write to I2CCR only
>> after it sees SCL idle for 64K cycle of internal I2C controller clocks. If
>> during this waiting period, I2C controller is disabled (I2CCR[MEN] set to
>> 0), then the controller could end in bad state, and hang the future access
>> to I2C register.
>>
>> The mpc_i2c_fixup() function tries to recover the bus from a stalled state
>> where the 9th clock pulse wasn't generated. However, this workaround
>> disables and enables I2C controller without meeting waiting requirement of
>> this erratum.
>>
>> This erratum applies to some 85xx SoCs. It is safe to apply to all of them
>> for mpc_i2c_fixup().
>>
>> Signed-off-by: York Sun <yorksun@freescale.com>
>>
>> ---
>> I'd like to get rid of the #ifdef if mpc5121 is OK with the longer delay.
>
> Are mpc5121 and mpc85xx the only things that use this?
No. 83xx and 86xx also uses this file. But I am only unsure if mpc52xx
is OK with this extended delay. I guess they are but I don't have a
proof, or someone to confirm.
>
> Are you sure the delay always works out to be longer? What is the
> relationship between fsl_get_sys_freq() and i2c->real_clk?
Yes. The max divider from sys clock to i2c clcok is 32K. i2c->real_clk
is the clock I2C controller pumps out, not its internal operation clock.
>
> In any case, you should send this patch to the i2c maintainer and list.
>
I don't have the name on top of my head. Is that linux-i2c@vger.kernel.org?
York
^ permalink raw reply
* [PATCH v2] Fix a typo in pSeries_lpar_hpte_insert()
From: Denis Kirjanov @ 2013-07-23 11:28 UTC (permalink / raw)
To: linuxppc-dev; +Cc: Denis Kirjanov, aneesh.kumar
Commit 801eb73f45371accc78ca9d6d22d647eeb722c11 introduced
a bug while checking PTE flags. We have to drop the _PAGE_COHERENT flag
when __PAGE_NO_CACHE is set and the cache update policy is not write-through
(i.e. _PAGE_WRITETHRU is not set)
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
CC: Michael Ellerman <michael@ellerman.id.au>
---
v1->v2:
- Updated commit log
---
arch/powerpc/platforms/pseries/lpar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
index 02d6e21..8bad880 100644
--- a/arch/powerpc/platforms/pseries/lpar.c
+++ b/arch/powerpc/platforms/pseries/lpar.c
@@ -146,7 +146,7 @@ static long pSeries_lpar_hpte_insert(unsigned long hpte_group,
flags = 0;
/* Make pHyp happy */
- if ((rflags & _PAGE_NO_CACHE) & !(rflags & _PAGE_WRITETHRU))
+ if ((rflags & _PAGE_NO_CACHE) && !(rflags & _PAGE_WRITETHRU))
hpte_r &= ~_PAGE_COHERENT;
if (firmware_has_feature(FW_FEATURE_XCMO) && !(hpte_r & HPTE_R_N))
flags |= H_COALESCE_CAND;
--
1.8.0.2
^ permalink raw reply related
* Re: [PATCH 1/3] cpuidle/powernv: cpuidle backend driver for powernv
From: Michael Ellerman @ 2013-07-23 14:06 UTC (permalink / raw)
To: Deepthi Dharwar; +Cc: linux-pm, linuxppc-dev, linux-kernel
In-Reply-To: <20130723090137.7291.36657.stgit@deepthi.in.ibm.com>
On Tue, Jul 23, 2013 at 02:31:41PM +0530, Deepthi Dharwar wrote:
> This patch implements a back-end cpuidle driver for
> powernv calling power7_nap and snooze idle states.
> This can be extended by adding more idle states
> in the future to the existing framework.
Other than the state table and a few minor details this looks almost
identical to the pseries driver. Can we not have a single version in
sysdev and isolate just the differences?
cheers
^ permalink raw reply
* Re: cpuidle/pseries: Fix kernel command line parameter smt-snooze-delay
From: Michael Ellerman @ 2013-07-23 14:02 UTC (permalink / raw)
To: Deepthi Dharwar
Cc: Preeti Murthy, PowerPC email list, linux-kernel@vger.kernel.org,
Srivatsa S. Bhat
In-Reply-To: <51EE0C65.6020903@linux.vnet.ibm.com>
On Tue, Jul 23, 2013 at 10:23:57AM +0530, Deepthi Dharwar wrote:
> smt-snooze-delay is a tun-able provided currently on powerpc to delay the
> entry of an idle cpu to NAP state. By default, the value is 100us,
> which is entry criteria for NAP state i.e only if the idle period is
> above 100us it would enter NAP. Value of -1 disables entry into NAP.
> This value can be set either through sysfs, ppc64_cpu util or by
> passing it via kernel command line. Currently this feature is broken
> when the value is passed via the kernel command line.
Has it always been broken? Or just since some commit?
> This patch aims to fix this, by taking the appropriate action
> based on the value after the pseries driver is registered.
> This check is carried on in the back-end driver rather than
> setup_smt_snooze_delay(), as one is not sure if the cpuidle driver
> is even registered when setup routine is executed.
> Also, this fixes re-enabling of NAP states by setting appropriate
> value without having to reboot using smt-snooze-delay parameter.
>
> Also, to note is, smt-snooze-delay is per-cpu variable.
> This can be used to enable/disable NAP on per-cpu
> basis using sysfs but when this variable is passed
> via kernel command line or using the smt-snooze-delay
> it applies to all the cpus. Per-cpu tuning can
> only be done via sysfs.
>
> Signed-off-by: Deepthi Dharwar <deepthi@linux.vnet.ibm.com>
> ---
> arch/powerpc/platforms/pseries/processor_idle.c | 34 ++++++++++++++++++-----
> 1 file changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/processor_idle.c b/arch/powerpc/platforms/pseries/processor_idle.c
> index 4644efa0..8133f50 100644
> --- a/arch/powerpc/platforms/pseries/processor_idle.c
> +++ b/arch/powerpc/platforms/pseries/processor_idle.c
> @@ -170,18 +170,36 @@ static struct cpuidle_state shared_states[MAX_IDLE_STATE_COUNT] = {
> void update_smt_snooze_delay(int cpu, int residency)
> {
> struct cpuidle_driver *drv = cpuidle_get_driver();
> - struct cpuidle_device *dev = per_cpu(cpuidle_devices, cpu);
> + struct cpuidle_device *dev;
>
> if (cpuidle_state_table != dedicated_states)
> return;
>
> - if (residency < 0) {
> - /* Disable the Nap state on that cpu */
> - if (dev)
> - dev->states_usage[1].disable = 1;
> - } else
> - if (drv)
> + if (!drv)
> + return;
> +
> + if (cpu == -1) {
> + if (residency < 0) {
> + /* Disable NAP on all cpus */
> + drv->states[1].disabled = true;
> + } else {
> drv->states[1].target_residency = residency;
> + drv->states[1].disabled = false;
> + }
> + return;
> + }
> +
> + dev = per_cpu(cpuidle_devices, cpu);
> + if (!dev)
> + return;
> +
> + if (residency < 0)
> + dev->states_usage[1].disable = 1;
> + else {
> + drv->states[1].target_residency = residency;
> + drv->states[1].disabled = false;
> + dev->states_usage[1].disable = 0;
Why are we indexing into all these array with '1' ?
cheers
^ permalink raw reply
* Re: [Suggestion] powerpc: xmon: about 'longjmp' related warning.
From: Michael Ellerman @ 2013-07-23 13:58 UTC (permalink / raw)
To: Chen Gang
Cc: Michael Neuling, paulus@samba.org, Andrew Morton,
linuxppc-dev@lists.ozlabs.org, jovi.zhangwei
In-Reply-To: <51ECD91D.7090903@asianux.com>
On Mon, Jul 22, 2013 at 03:02:53PM +0800, Chen Gang wrote:
> Hello Maintainers:
>
> With allmodconfig and EXTRA_CFLAGS=-W", it reports warnings below:
>
> arch/powerpc/xmon/xmon.c:3027:6: warning: variable ‘i’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
> arch/powerpc/xmon/xmon.c:3068:6: warning: variable ‘i’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
In both these cases we are inside the body of a for loop and we do a
if (setjmp) / else block. Although looking at the source the value of i
is not modified by the setjmp, I guess it's possible that the compiler
might reorder the increment of i inside the setjmp and loose the value
when we longjmp.
> arch/powerpc/xmon/xmon.c:352:48: warning: argument ‘fromipi’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
This one I can't see, but I assume it's a similar case.
> Excuse me, I am not quite sure about it whether can cause issue or not.
I've never seen it get stuck in those loops or anything, but I guess
it's possible.
The first thing to do would be to analyse the generated assembler code
to determine if there really is any possiblity of the value being
clobbered, or if it's just a theoretical bug.
cheers
^ permalink raw reply
* Re: [PATCH v2] powerpc: kernel: remove useless code which related with 'max_cpus'
From: Michael Ellerman @ 2013-07-23 13:44 UTC (permalink / raw)
To: Srivatsa S. Bhat
Cc: linuxppc-dev@lists.ozlabs.org, Chen Gang, paulus@samba.org,
chenhui.zhao, Thomas Gleixner
In-Reply-To: <51ECD664.7040708@linux.vnet.ibm.com>
On Mon, Jul 22, 2013 at 12:21:16PM +0530, Srivatsa S. Bhat wrote:
> On 07/22/2013 12:10 PM, Chen Gang wrote:
> > Since not need 'max_cpus' after the related commit, the related code
> > are useless too, need be removed.
> >
> > The related commit:
> >
> > c1aa687 powerpc: Clean up obsolete code relating to decrementer and timebase
> >
> > The related warning:
> >
> > arch/powerpc/kernel/smp.c:323:43: warning: parameter ‘max_cpus’ set but not used [-Wunused-but-set-parameter]
> >
> > Signed-off-by: Chen Gang <gang.chen@asianux.com>
>
> This version looks good.
Agreed.
A good follow up patch, or actually series of patches, would be to
change the prototype of smp_ops->probe() to return void, and fix all the
implementations to no longer return anything.
cheers
^ permalink raw reply
* Re: [PATCH] Fix a typo in pSeries_lpar_hpte_insert()
From: Michael Ellerman @ 2013-07-23 13:42 UTC (permalink / raw)
To: Aneesh Kumar K.V; +Cc: Denis Kirjanov, linuxppc-dev
In-Reply-To: <874nbmobww.fsf@linux.vnet.ibm.com>
On Mon, Jul 22, 2013 at 08:54:31PM +0530, Aneesh Kumar K.V wrote:
> Denis Kirjanov <kda@linux-powerpc.org> writes:
>
> > Fix a typo in pSeries_lpar_hpte_insert()
> >
> > Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>
> looks good
>
> Reviewed-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
>
> We may want to add the commit that introduced the change ?
>
> 801eb73f45371accc78ca9d6d22d647eeb722c11
Yes, as well as describe what the typo is, and what the implications
are - ie. what was broken and is now fixed.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/8xx: Remove last traces of 8XX_MINIMAL_FPEMU
From: Michael Ellerman @ 2013-07-23 13:38 UTC (permalink / raw)
To: Paul Bolle; +Cc: Paul Mackerras, linuxppc-dev, linux-kernel
In-Reply-To: <1373964998.2591.57.camel@x61.thuisdomein>
On Tue, Jul 16, 2013 at 10:56:38AM +0200, Paul Bolle wrote:
> The Kconfig symbol 8XX_MINIMAL_FPEMU was removed in commit 968219fa33
> ("powerpc/8xx: Remove 8xx specific "minimal FPU emulation""). But that
> commit didn't remove all code depending on that symbol. Do so now.
>
> Signed-off-by: Paul Bolle <pebolle@tiscali.nl>
> ---
> Not even compile tested!
Help yourself to a cross compiler :)
https://www.kernel.org/pub/tools/crosstool/
cheers
^ permalink raw reply
* Re: [v1][PATCH 1/1] powerpc/ppc64: rename SOFT_DISABLE_INTS with RECONCILE_IRQ_STATE
From: Michael Ellerman @ 2013-07-23 13:37 UTC (permalink / raw)
To: Tiejun Chen; +Cc: scottwood, kvm-ppc, kvm, linuxppc-dev
In-Reply-To: <1373944170-22559-1-git-send-email-tiejun.chen@windriver.com>
On Tue, Jul 16, 2013 at 11:09:30AM +0800, Tiejun Chen wrote:
> The SOFT_DISABLE_INTS seems an odd name for something that updates the
> software state to be consistent with interrupts being hard disabled, so
> rename SOFT_DISABLE_INTS with RECONCILE_IRQ_STATE to avoid this confusion.
Yes!
I have a similar but unfinished patch locally.
cheers
^ permalink raw reply
* Re: [PATCH] module: ppc64 module CRC relocation fix causes perf issues
From: Michael Ellerman @ 2013-07-23 13:30 UTC (permalink / raw)
To: Scott Wood
Cc: Neil Horman, Rusty Russell, linux-kernel, Paul Mackerras,
Anton Blanchard, linuxppc-dev
In-Reply-To: <1374274770.5357.33@snotra>
On Fri, Jul 19, 2013 at 05:59:30PM -0500, Scott Wood wrote:
> On 07/17/2013 11:00:45 PM, Anton Blanchard wrote:
> >
> >Hi Scott,
> >
> >> What specifically should I do to test it?
> >
> >Could you double check perf annotate works? I'm 99% sure it will but
> >that is what was failing on ppc64.
>
> I'm not really sure what it's supposed to look like when "perf
> annotate" works. It spits a bunch of unreadable[1]
> dark-blue-on-black assembly code at me, all with "0.00 :" in the
> left column.
>
> Oh, wait -- some lines have "100.00 : " on the left, in
> even-more-unreadable dark-red-on-black.
>
> Apart from the annoying colors, is there anything specific I should
> be looking for? Some sort of error message, or output that actually
> makes sense?
The colours look fine on my terminal, so I don't know what you've done
there. If you care you can use "--stdio" to use the plainer interface,
though it still uses colours.
That output looks fine in terms of the bug Anton was chasing. As far as
only ever hitting one instruction that does look weird.
cheers
^ permalink raw reply
* Re: [PATCH V3 2/2] powerpc, perf: BHRB filter configuration should follow the task
From: Michael Ellerman @ 2013-07-23 13:20 UTC (permalink / raw)
To: Benjamin Herrenschmidt; +Cc: linuxppc-dev, Michael Neuling, Anshuman Khandual
In-Reply-To: <1374475696.3916.35.camel@pasglop>
On Mon, Jul 22, 2013 at 04:48:16PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2013-07-22 at 12:09 +0530, Anshuman Khandual wrote:
> > These patches have not been applied yet. Would like me to correct
> > these spellings myself and send out again. Please do let me know.
> > Thanks !
>
> No, it's ok, I'll add them to my batch.
>
> Usually I assume that perf related stuff goes through acme's perf tree
> so I might have overlooked them because of that.
>
> Michael, you're ok with these ?
Which Michael? :)
I haven't had time to really get my head around the BHRB code, so I
can't really say.
cheers
^ permalink raw reply
* Re: [PATCH v3 17/31] clk: mpc512x: introduce COMMON_CLK for MPC512x
From: Gerhard Sittig @ 2013-07-23 13:14 UTC (permalink / raw)
To: linuxppc-dev, Anatolij Gustschin, Mike Turquette,
linux-arm-kernel, devicetree
Cc: Detlev Zundel, Wolfram Sang, Greg Kroah-Hartman, Rob Herring,
Mark Brown, Marc Kleine-Budde, David Woodhouse,
Wolfgang Grandegger, Mauro Carvalho Chehab
In-Reply-To: <1374495298-22019-18-git-send-email-gsi@denx.de>
[ summary: "shared gate" support desirable? approach acceptable? ]
On Mon, Jul 22, 2013 at 14:14 +0200, Gerhard Sittig wrote:
>
> this change implements a clock driver for the MPC512x PowerPC platform
> which follows the COMMON_CLK approach and uses common clock drivers
> shared with other platforms
>
> [ ... ]
>
> some of the clock items get pre-enabled in the clock driver to not have
> them automatically disabled by the underlying clock subsystem because of
> their being unused -- this approach is desirable because
> [ ... ]
> - some help introduce support for and migrate to the common
> infrastructure, while more appropriate support for specific hardware
> constraints isn't available yet (remaining changes are strictly
> internal to the clock driver and won't affect peripheral drivers)
This remark was related to the CAN clocks of the MPC512x SoC.
The clock subtrees which are involved in generating CAN bitrates
include one path from the XTAL to an internal MCLK (this is part
of the CCF support for the platform), and another path from the
MCLK or yet another IP bus clock to the actual bitrate on the
wire (this is taken care of within the mscan(4) driver).
The MCLK generation for CAN is documented in the MPC5121e
Reference Manual, chapter 5, section 5.2.5 "MSCAN Clock
Generation". SYS, REF (both internal), PSC_MCLK_IN, and SPDIF_TX
(both external) are muxed, gated, and divided. The result is
muxed with IP. The result is fed into the MSCAN component and
gets muxed with IP again (can't tell why, maybe for backwards
compatibility).
In parallel to this MCLK block there is SCCR2[25], the "BDLC and
MSCAN clock enable", documented in section 5.3.1.3 "System Clock
Control Register 2". So there is a gate that "somehow needs to
get setup" yet isn't part of the visible MCLK chain.
The series up to and including v3 approaches the problem by
- adding a gate after the second MCLK mux, which gets exported
for client lookups and is the MCLK input for the mscan(4)
driver
- creating that gate for each of the four MSCAN clocks of the
SoC, all of them referencing the single "enable" bit in the
SCCR2 register
- pre-enabling the MSCAN clocks from within the clock driver, and
thus avoid having the clock disabled from the common
infrastructure, because disabling one of these clocks had
closed the shared gate and thus had broken all other clock uses
> clkdev registration provides "alias names" for few clock items
> [ ... ]
>
[ ... ]
> +
> +/* setup the MCLK clock subtree of an individual PSC/MSCAN/SPDIF */
> +static void mpc512x_clk_setup_mclk(struct mclk_setup_data *entry)
> +{
> + size_t clks_idx_pub, clks_idx_int;
> + u32 __iomem *mccr_reg; /* MCLK control register (mux, en, div) */
> + u32 __iomem *sccr_reg; /* system clock control register (enable) */
> + int sccr_bit;
> + int div;
> +
> + /* derive a few parameters from the component type and index */
> + switch (entry->type) {
> + case MCLK_TYPE_PSC:
> + clks_idx_pub = MPC512x_CLK_PSC0_MCLK + entry->comp_idx;
> + clks_idx_int = MPC512x_CLK_MCLKS_FIRST
> + + (entry->comp_idx) * MCLK_MAX_IDX;
> + mccr_reg = &clkregs->psc_ccr[entry->comp_idx];
> + break;
> + case MCLK_TYPE_MSCAN:
> + clks_idx_pub = MPC512x_CLK_MSCAN0_MCLK + entry->comp_idx;
> + clks_idx_int = MPC512x_CLK_MCLKS_FIRST
> + + (NR_PSCS + entry->comp_idx) * MCLK_MAX_IDX;
> + mccr_reg = &clkregs->mscan_ccr[entry->comp_idx];
> + break;
> + case MCLK_TYPE_SPDIF:
> + clks_idx_pub = MPC512x_CLK_SPDIF_MCLK;
> + clks_idx_int = MPC512x_CLK_MCLKS_FIRST
> + + (NR_PSCS + NR_MSCANS) * MCLK_MAX_IDX;
> + mccr_reg = &clkregs->spccr;
> + break;
> + default:
> + return;
> + }
> + if (entry->bit_sccr1 >= 0) {
> + sccr_reg = &clkregs->sccr1;
> + sccr_bit = entry->bit_sccr1;
> + } else if (entry->bit_sccr2 >= 0) {
> + sccr_reg = &clkregs->sccr2;
> + sccr_bit = entry->bit_sccr2;
> + } else {
> + sccr_reg = NULL;
> + }
> +
> + /*
> + * this was grabbed from the PPC_CLOCK implementation, which
> + * enforced a specific MCLK divider while the clock was gated
> + * during setup (that's a documented hardware requirement)
> + *
> + * the PPC_CLOCK implementation might even have violated the
> + * "MCLK <= IPS" constraint, the fixed divider value of 1
> + * results in a divider of 2 and thus MCLK = SYS/2 which equals
> + * CSB which is greater than IPS; the serial port setup may have
> + * adjusted the divider which the clock setup might have left in
> + * an undesirable state
> + *
> + * initial setup is:
> + * - MCLK 0 from SYS
> + * - MCLK DIV such to not exceed the IPS clock
> + * - MCLK 0 enabled
> + * - MCLK 1 from MCLK DIV
> + */
> + div = clk_get_rate(clks[MPC512x_CLK_SYS]);
> + div /= clk_get_rate(clks[MPC512x_CLK_IPS]);
> + out_be32(mccr_reg, (0 << 16));
> + out_be32(mccr_reg, (0 << 16) | ((div - 1) << 17));
> + out_be32(mccr_reg, (1 << 16) | ((div - 1) << 17));
> +
> + /*
> + * create the 'struct clk' items of the MCLK's clock subtree
> + *
> + * note that by design we always create all nodes and won't take
> + * shortcuts here, because
> + * - the "internal" MCLK_DIV and MCLK_OUT signal in turn are
> + * selectable inputs to the CFM while those who "actually use"
> + * the PSC/MSCAN/SPDIF (serial drivers et al) need the MCLK
> + * for their bitrate
> + * - in the absence of "aliases" for clocks we need to create
> + * individial 'struct clk' items for whatever might get
> + * referenced or looked up, even if several of those items are
> + * identical from the logical POV (their rate value)
> + * - for easier future maintenance and for better reflection of
> + * the SoC's documentation, it appears appropriate to generate
> + * clock items even for those muxers which actually are NOPs
> + * (those with two inputs of which one is reserved)
> + */
> + clks[clks_idx_int + MCLK_IDX_MUX0] = mpc512x_clk_muxed(
> + entry->name_mux0,
> + &parent_names_mux0[0], ARRAY_SIZE(parent_names_mux0),
> + mccr_reg, 14, 2);
> + clks[clks_idx_int + MCLK_IDX_EN0] = mpc512x_clk_gated(
> + entry->name_en0, entry->name_mux0,
> + mccr_reg, 16);
> + clks[clks_idx_int + MCLK_IDX_DIV0] = mpc512x_clk_divider(
> + entry->name_div0,
> + entry->name_en0, CLK_SET_RATE_GATE,
> + mccr_reg, 17, 15, 0);
> + if (entry->has_mclk1) {
> + clks[clks_idx_int + MCLK_IDX_MUX1] = mpc512x_clk_muxed(
> + entry->name_mux1,
> + &entry->parent_names_mux1[0],
> + ARRAY_SIZE(entry->parent_names_mux1),
> + mccr_reg, 7, 1);
> + } else {
> + clks[clks_idx_int + MCLK_IDX_MUX1] = mpc512x_clk_factor(
> + entry->name_mux1, entry->parent_names_mux1[0],
> + 1, 1);
> + }
> + if (sccr_reg) {
> + clks[clks_idx_pub] = mpc512x_clk_gated(
> + entry->name_mclk,
> + entry->name_mux1, sccr_reg, sccr_bit);
> + } else {
> + clks[clks_idx_pub] = mpc512x_clk_factor(
> + entry->name_mclk,
> + entry->name_mux1, 1, 1);
> + }
> +
> + /*
> + * without this "clock device" registration, "simple" lookups in
> + * the SPI master initialization and serial port setup will fail
> + *
> + * those drivers need to get adjusted to lookup their required
> + * clocks from device tree specs, and device tree nodes need to
> + * provide the clock specs, before this clkdev registration
> + * becomes obsolete
> + */
> + clk_register_clkdev(clks[clks_idx_pub], entry->name_mclk, NULL);
> +}
> [ ... ]
This was the routine which sets up _one_ MCLK block, note the
assignment at the routine's end to the "published" clock item
that's the gate's output after the second mux stage.
> [ ... ]
> + clks[MPC512x_CLK_I2C] = mpc512x_clk_gated("i2c", "ips",
> + &clkregs->sccr2, 26);
> + mpc512x_clk_setup_mclks(mclk_mscan_data, ARRAY_SIZE(mclk_mscan_data));
> + clks[MPC512x_CLK_SDHC] = mpc512x_clk_gated("sdhc", "sdhc-ug",
> + &clkregs->sccr2, 24);
> + mpc512x_clk_setup_mclks(mclk_spdif_data, ARRAY_SIZE(mclk_spdif_data));
> [ ... ]
This is the invocation of the routine which sets up four MCLK
blocks for the MSCAN components, while all of them refer to bit
25 of SCCR2.
> [ ... ]
> +
> + /* enable some of the clocks here unconditionally because ... */
> + pr_debug("automatically enabling some clocks\n");
> + /* some are essential yet never get claimed by any driver */
> + clk_prepare_enable(clks[MPC512x_CLK_DUMMY]);
> + clk_prepare_enable(clks[MPC512x_CLK_E300]); /* PowerPC CPU */
> + clk_prepare_enable(clks[MPC512x_CLK_DDR]); /* DRAM */
> + clk_prepare_enable(clks[MPC512x_CLK_MEM]); /* SRAM */
> + clk_prepare_enable(clks[MPC512x_CLK_IPS]); /* SoC periph */
> + clk_prepare_enable(clks[MPC512x_CLK_LPC]); /* boot media */
> + /* some are required yet no dependencies were declared */
> + clk_prepare_enable(clks[MPC512x_CLK_PSC_FIFO]);
> + /* some are not yet acquired by their respective drivers */
> + clk_prepare_enable(clks[MPC512x_CLK_PSC3_MCLK]);/* serial console */
> + clk_prepare_enable(clks[MPC512x_CLK_FEC]); /* network, NFS */
> + clk_prepare_enable(clks[MPC512x_CLK_DIU]); /* display */
> + clk_prepare_enable(clks[MPC512x_CLK_I2C]);
> + /*
> + * some have their individual clock subtree with separate clock
> + * items and their individual enable counters, yet share a
> + * common gate (refer to the same register location) while the
> + * common clock driver code is not aware of the fact and the
> + * platform's code doesn't provide specific support either
> + *
> + * what might happen is that e.g. enabling two MSCAN clock items
> + * and disabling one of them will disable the common gate and
> + * thus break the other MSCAN clock as well
> + */
> + clk_prepare_enable(clks[MPC512x_CLK_MSCAN0_MCLK]);
> + clk_prepare_enable(clks[MPC512x_CLK_MSCAN1_MCLK]);
> + clk_prepare_enable(clks[MPC512x_CLK_MSCAN2_MCLK]);
> + clk_prepare_enable(clks[MPC512x_CLK_MSCAN3_MCLK]);
> +}
This is the pre-enable workaround for the MSCAN0 to MSCAN3 clock
items.
The above approach does work in that it introduces complete
support for common clock on the MPC512x platform, with the CAN
component being operational, and the clock driver using shared
logic across platforms.
The remaining issue is that regardless of whether CAN is used,
the (chip internal) clock is enabled. This may not be a problem
when bitrates aren't generated and the wire isn't driven.
The question now is how to correctly support the situation where
a gate is shared between subtrees yet isn't really part of any
path within the subtrees. I really cannot find a single spot
where to introduce the gate such that it's not duplicated.
The appropriate solution would not be to pre-enable those clocks,
but to either introduce another gate clock type which supports a
shared reference, or to add support for the shared reference to
the existing gate code.
I'd rather not duplicate most or all of the code of clk-gate.c,
instead I looked into how to add "shared gate" support to the
existing driver.
My question is whether the approach is acceptable. It adds
minimal overhead and shall be OK for the enable/disable path from
a technical POV. And it doesn't feel like too much of a stretch.
But there may be non-technical reasons to reject the approach.
I'd like to learn whether to follow that path before preparing
another version of the patch series.
The diffs were taken with the '-w -b' options to demonstrate
their essence and not drown it in whitespace changes. The
implementation assumes that the caller which registers the gate
(the platform's clock driver) provides both the counter cell and
the lock. And that all gates with a "shared use counter" use the
same lock (which is satisfied as they all get registered from the
same spot in the platform's clock driver).
The CLK_IGNORE_UNUSED flag addresses a different problem. The
SoC has four MSCAN components, while two of them are enabled in
the device tree (the other two are present but disabled). So
during probe two of the clocks get enabled. After probe all
unused clocks automatically get disabled (that's another two).
So the "shared use counter" drops to zero although components are
in use, because "disable, it's unused" isn't told from "disable
after enable, regular use". The flag would become obsolete if
the common gate logic would implement a separate disable_unused()
routine, but I guess this isn't necessary and the use of the flag
is appropriate.
That the example use creates a field for just one counter is to
better demonstrate the use and potential extension as need
arises. Reducing this to a mere integer variable would be a
micro optimization.
The extension of the existing clk_gate implementation:
--- a/drivers/clk/clk-gate.c
+++ b/drivers/clk/clk-gate.c
@@ -46,6 +46,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
struct clk_gate *gate = to_clk_gate(hw);
int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
unsigned long flags = 0;
+ int need_reg_access;
u32 reg;
set ^= enable;
@@ -53,6 +54,20 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
if (gate->lock)
spin_lock_irqsave(gate->lock, flags);
+ /*
+ * if a "shared use counter" was specified, keep track of enable
+ * and disable calls and only access hardware registers upon the
+ * very first enable or very last disable call
+ */
+ if (!gate->share_count) {
+ need_reg_access = 1;
+ } else if (enable) {
+ need_reg_access = (*gate->share_count)++ == 0;
+ } else {
+ need_reg_access = --(*gate->share_count) == 0;
+ }
+
+ if (need_reg_access) {
if (gate->flags & CLK_GATE_HIWORD_MASK) {
reg = BIT(gate->bit_idx + 16);
if (set)
@@ -67,6 +82,7 @@ static void clk_gate_endisable(struct clk_hw *hw, int enable)
}
clk_writel(reg, gate->reg);
+ }
if (gate->lock)
spin_unlock_irqrestore(gate->lock, flags);
@@ -118,10 +134,11 @@ EXPORT_SYMBOL_GPL(clk_gate_ops);
* @clk_gate_flags: gate-specific flags for this clock
* @lock: shared register lock for this clock
*/
-struct clk *clk_register_gate(struct device *dev, const char *name,
+struct clk *clk_register_gate_shared(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 bit_idx,
- u8 clk_gate_flags, spinlock_t *lock)
+ u8 clk_gate_flags, spinlock_t *lock,
+ int *share_count)
{
struct clk_gate *gate;
struct clk *clk;
@@ -152,6 +169,7 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
gate->bit_idx = bit_idx;
gate->flags = clk_gate_flags;
gate->lock = lock;
+ gate->share_count = share_count;
gate->hw.init = &init;
clk = clk_register(dev, &gate->hw);
@@ -161,3 +179,14 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
return clk;
}
+
+struct clk *clk_register_gate(struct device *dev, const char *name,
+ const char *parent_name, unsigned long flags,
+ void __iomem *reg, u8 bit_idx,
+ u8 clk_gate_flags, spinlock_t *lock)
+{
+
+ return clk_register_gate_shared(dev, name, parent_name, flags,
+ reg, bit_idx, clk_gate_flags,
+ lock, NULL);
+}
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -222,6 +222,7 @@ struct clk_gate {
u8 bit_idx;
u8 flags;
spinlock_t *lock;
+ int *share_count;
};
#define CLK_GATE_SET_TO_DISABLE BIT(0)
@@ -232,6 +233,11 @@ struct clk *clk_register_gate(struct device *dev, const char *name,
const char *parent_name, unsigned long flags,
void __iomem *reg, u8 bit_idx,
u8 clk_gate_flags, spinlock_t *lock);
+struct clk *clk_register_gate_shared(struct device *dev, const char *name,
+ const char *parent_name, unsigned long flags,
+ void __iomem *reg, u8 bit_idx,
+ u8 clk_gate_flags, spinlock_t *lock,
+ int *share_count);
struct clk_div_table {
unsigned int val;
How to use these shared gates:
--- a/arch/powerpc/platforms/512x/clock-commonclk.c
+++ b/arch/powerpc/platforms/512x/clock-commonclk.c
@@ -123,6 +123,39 @@ static inline struct clk *mpc512x_clk_gated(
reg, pos, 0, &clklock);
}
+enum mpc512x_clk_shared_gate_id_t {
+ MPC512x_CLK_SHARED_GATE_MSCAN,
+ MPC512x_CLK_SHARED_GATE_MAX,
+};
+
+static int mpc512x_clk_gate_counters[MPC512x_CLK_SHARED_GATE_MAX];
+
+/*
+ * implementor's note: since clk_gate items don't implement a separate
+ * .disable_unused() callback, their .disable() routine gets called and
+ * "disable the clock as we can't see it's in use" cannot be told from
+ * "regular disable, count these events please"
+ *
+ * passing the CLK_IGNORE_UNUSED flag upon clock creation will suppress
+ * the "disable, unused" call, so use counts won't get unbalanced, the
+ * clock either never got enabled and thus need not get disabled, or
+ * part of the hardware got enabled while disabling the other part isn't
+ * wanted
+ */
+static inline struct clk *mpc512x_clk_gated_shared(
+ const char *name, const char *parent_name,
+ u32 __iomem *reg, u8 pos,
+ enum mpc512x_clk_shared_gate_id_t share_id)
+{
+ int clkflags;
+
+ clkflags = CLK_SET_RATE_PARENT;
+ clkflags |= CLK_IGNORE_UNUSED;
+ return clk_register_gate_shared(NULL, name, parent_name, clkflags,
+ reg, pos, 0, &clklock,
+ &mpc512x_clk_gate_counters[share_id]);
+}
+
static inline struct clk *mpc512x_clk_muxed(const char *name,
const char **parent_names, int parent_count,
u32 __iomem *reg, u8 pos, u8 len)
@@ -520,9 +553,16 @@ static void mpc512x_clk_setup_mclk(struct mclk_setup_data *entry)
1, 1);
}
if (sccr_reg) {
+ if (entry->type == MCLK_TYPE_MSCAN) {
+ clks[clks_idx_pub] = mpc512x_clk_gated_shared(
+ entry->name_mclk,
+ entry->name_mux1, sccr_reg, sccr_bit,
+ MPC512x_CLK_SHARED_GATE_MSCAN);
+ } else {
clks[clks_idx_pub] = mpc512x_clk_gated(
entry->name_mclk,
entry->name_mux1, sccr_reg, sccr_bit);
+ }
} else {
clks[clks_idx_pub] = mpc512x_clk_factor(
entry->name_mclk,
Local tests have shown that the extension solves the problem of
how to satisfy the SoC's constraints on the MPC512x platform.
The MSCAN clocks no longer need to get pre-enabled, instead they
get setup and enabled only as the mscan(4) driver probes devices
according to how it was instructed (device tree nodes).
What do you think? Is the "shared gate" support in the common
logic appropriate? I'd rather not duplicate all of this code
just to introduce the specific gate I need, while most of the
logic is identical to the existing gate implementation. The
desire isn't to override the gate's operations, but to wrap them
and to consult a counter in addition, while the register access
still applies.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: [PATCH v3 27/31] net: can: mscan: add common clock support for mpc512x
From: Marc Kleine-Budde @ 2013-07-23 12:37 UTC (permalink / raw)
To: linuxppc-dev, Anatolij Gustschin, Mike Turquette,
linux-arm-kernel, devicetree, Wolfram Sang, Mauro Carvalho Chehab,
David Woodhouse, Wolfgang Grandegger, Pantelis Antoniou,
Mark Brown, Greg Kroah-Hartman, Rob Herring, Detlev Zundel
In-Reply-To: <20130723120733.GH19071@book.gsilab.sittig.org>
[-- Attachment #1: Type: text/plain, Size: 1255 bytes --]
On 07/23/2013 02:07 PM, Gerhard Sittig wrote:
[...]
>>> + return freq_calc;
>>> +
>>> +err_invalid:
>>> + dev_err(&ofdev->dev, "invalid clock source specification\n");
>>> + return 0;
>>
>> return 0 in case of error? Please add a comment what this 0 means here.
>
> The .get_clock() callback is supposed to return the resulting
> rate after the clock source was determined and the clock subtree
> was setup. It wasn't (explicitly) documented before (in the
> non-common-clock case), so I did not bother to comment it in the
> parallel common-clock case.
Yes, but returning 0 in case of error should be documented, as it's very
uncommon.
> But it's true that returning zero in the error case may be
> unexpected, and I will add a comment in v4 (in all the
> .get_clock() implementations).
I think a one liner stating that the return value is the clock rate and
"0" indicates is an invalid clock rate, this meaning an error here, is
enough.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]
^ permalink raw reply
* Re: [PATCH v3 11/31] net: can: mscan: improve clock API use
From: Marc Kleine-Budde @ 2013-07-23 12:33 UTC (permalink / raw)
To: linuxppc-dev, Anatolij Gustschin, Mike Turquette,
linux-arm-kernel, devicetree-discuss, Wolfram Sang,
Mauro Carvalho Chehab, David Woodhouse, Wolfgang Grandegger,
Pantelis Antoniou, Mark Brown, Greg Kroah-Hartman, Rob Herring,
Detlev Zundel
In-Reply-To: <20130723115348.GF19071@book.gsilab.sittig.org>
[-- Attachment #1: Type: text/plain, Size: 3732 bytes --]
On 07/23/2013 01:53 PM, Gerhard Sittig wrote:
> On Mon, Jul 22, 2013 at 14:31 +0200, Marc Kleine-Budde wrote:
>>
>> On 07/22/2013 02:14 PM, Gerhard Sittig wrote:
>>> the .get_clock() callback is run from probe() and might allocate
>>> resources, introduce a .put_clock() callback that is run from remove()
>>> to undo any allocation activities
>>
>> looks good
>>
>>> use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put
>>> upon driver unload
>>
>> fine
>>
>>> assume that resources get prepared but not necessarily enabled in the
>>> setup phase, make the open() and close() callbacks of the CAN network
>>> device enable and disable a previously acquired and prepared clock
>>
>> I think you should call prepare_enable and disable_unprepare in the
>> open/close functions.
>
> After more local research, which totally eliminated the need to
> pre-enable the CAN related clocks, but might need more discussion
> as it touches the common gate support, I've learned something
> more:
>
> The CAN clock needs to get enabled during probe() already, since
> registers get accessed between probe() for the driver and open()
> for the network device -- while access to peripheral registers
> crashes the kernel when clocks still are disabled (other hardware
> may just hang or provide fake data, neither of this is OK).
Then call prepare_enable(); before and disable_unprepare(); after
accessing the registers. Have a look at the flexcan driver.
> But I see the point in your suggestion to prepare _and_ enable
> the clock during open() as well -- to have open() cope with
> whatever probe() did, after all the driver is shared among
> platforms, which may differ in what they do during probe().
If you enable a clock to access the registers before open() (and disable
it afterwards), it should not harm any architecture that doesn't need
this clock enabled.
> So I will:
> - make open() of the network device prepare _and_ enable the
> clock for the peripheral (if acquired during probe())
good
> - adjust open() because ATM it leaves the clock enabled when the
> network device operation fails (the error path is incomplete in
> v3)
yes, clock should be disabled if open() fails.
> - make the MPC512x specific probe() time .get_clock() routine not
> just prepare but enable the clock as well
If needed enable the clock, but disable after probe() has finished.
> - and of course address all the shutdown counter parts of the
> above setup paths
> This results in:
> - specific chip drivers only need to balance their private get
> and put clock routines which are called from probe and remove,
> common paths DTRT for all of them
Yes, but clock should not stay enabled between probe() and open().
[...]
> Removing unnecessary devm_put_clk() calls is orthogonal to that.
> Putting these in isn't totally wrong (they won't harm, and they
> do signal "visual balance" more clearly such that the next person
> won't stop and wonder), but it's true that they are redundant.
> "Trained persons" will wonder as much about their presence as
> untrained persons wonder about their absence. :) Apparently I'm
> not well trained yet.
The whole point about devm_* is to get rid of auto manually tear down
functions. So please remove all devm_put_clk() calls, as it will be
called automatically if a driver instance is removed.
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]
^ permalink raw reply
* [PATCH] powerpc: VPHN topology change updates all siblings
From: Robert Jennings @ 2013-07-23 12:33 UTC (permalink / raw)
To: linuxppc-dev
Cc: stable, Paul Mackerras, Nathan Fontenot, Robert Jennings,
Jan Stancek
When an associativity level change is found for one thread, the
siblings threads need to be updated as well. This is done today
for PRRN in stage_topology_update() but is missing for VPHN in
update_cpu_associativity_changes_mask().
All threads should be updated to move to the new node. Without this
patch, a single thread may be flagged for a topology change, leaving it
in a different node from its siblings, which is incorrect. This causes
problems for the scheduler where overlapping scheduler groups are created
and a loop is formed in those groups.
Reported-by: Jan Stancek <jstancek@redhat.com>
Signed-off-by: Robert Jennings <rcj@linux.vnet.ibm.com>
Cc: <stable@vger.kernel.org>
---
Looking to get this fix in 3.11.
This is requested for the 3.10 stable tree as well, it fixes a
bug that presents as a scheduler issue when VPHN is active.
VPHN was enabled in the v3.9 window in commit b7abef0.
---
arch/powerpc/mm/numa.c | 59
+++++++++++++++++++++++++++++++++++++-------------
1 file changed, 44 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
index 0839721..5850798 100644
--- a/arch/powerpc/mm/numa.c
+++ b/arch/powerpc/mm/numa.c
@@ -27,6 +27,7 @@
#include <linux/seq_file.h>
#include <linux/uaccess.h>
#include <linux/slab.h>
+#include <asm/cputhreads.h>
#include <asm/sparsemem.h>
#include <asm/prom.h>
#include <asm/smp.h>
@@ -1318,7 +1319,8 @@ static int update_cpu_associativity_changes_mask(void)
}
}
if (changed) {
- cpumask_set_cpu(cpu, changes);
+ cpumask_or(changes, changes, cpu_sibling_mask(cpu));
+ cpu = cpu_last_thread_sibling(cpu);
}
}
@@ -1426,7 +1428,7 @@ static int update_cpu_topology(void *data)
if (!data)
return -EINVAL;
- cpu = get_cpu();
+ cpu = smp_processor_id();
for (update = data; update; update = update->next) {
if (cpu != update->cpu)
@@ -1446,12 +1448,12 @@ static int update_cpu_topology(void *data)
*/
int arch_update_cpu_topology(void)
{
- unsigned int cpu, changed = 0;
+ unsigned int cpu, sibling, changed = 0;
struct topology_update_data *updates, *ud;
unsigned int associativity[VPHN_ASSOC_BUFSIZE] = {0};
cpumask_t updated_cpus;
struct device *dev;
- int weight, i = 0;
+ int weight, new_nid, i = 0;
weight = cpumask_weight(&cpu_associativity_changes_mask);
if (!weight)
@@ -1464,19 +1466,46 @@ int arch_update_cpu_topology(void)
cpumask_clear(&updated_cpus);
for_each_cpu(cpu, &cpu_associativity_changes_mask) {
- ud = &updates[i++];
- ud->cpu = cpu;
- vphn_get_associativity(cpu, associativity);
- ud->new_nid = associativity_to_nid(associativity);
-
- if (ud->new_nid < 0 || !node_online(ud->new_nid))
- ud->new_nid = first_online_node;
+ /*
+ * If siblings aren't flagged for changes, updates list
+ * will be too short. Skip on this update and set for next
+ * update.
+ */
+ if (!cpumask_subset(cpu_sibling_mask(cpu),
+ &cpu_associativity_changes_mask)) {
+ pr_info("Sibling bits not set for associativity "
+ "change, cpu%d\n", cpu);
+ cpumask_or(&cpu_associativity_changes_mask,
+ &cpu_associativity_changes_mask,
+ cpu_sibling_mask(cpu));
+ cpu = cpu_last_thread_sibling(cpu);
+ continue;
+ }
- ud->old_nid = numa_cpu_lookup_table[cpu];
- cpumask_set_cpu(cpu, &updated_cpus);
+ /* Use associativity from first thread for all siblings */
+ vphn_get_associativity(cpu, associativity);
+ new_nid = associativity_to_nid(associativity);
+ if (new_nid < 0 || !node_online(new_nid))
+ new_nid = first_online_node;
+
+ if (new_nid == numa_cpu_lookup_table[cpu]) {
+ cpumask_andnot(&cpu_associativity_changes_mask,
+ &cpu_associativity_changes_mask,
+ cpu_sibling_mask(cpu));
+ cpu = cpu_last_thread_sibling(cpu);
+ continue;
+ }
- if (i < weight)
- ud->next = &updates[i];
+ for_each_cpu(sibling, cpu_sibling_mask(cpu)) {
+ ud = &updates[i++];
+ ud->cpu = sibling;
+ ud->new_nid = new_nid;
+ ud->old_nid = numa_cpu_lookup_table[sibling];
+ cpumask_set_cpu(sibling, &updated_cpus);
+ if (i < weight)
+ ud->next = &updates[i];
+ }
+ cpu = cpu_last_thread_sibling(cpu);
}
stop_machine(update_cpu_topology, &updates[0], &updated_cpus);
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH v3 27/31] net: can: mscan: add common clock support for mpc512x
From: Gerhard Sittig @ 2013-07-23 12:07 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: devicetree, Mike Turquette, Detlev Zundel, Wolfram Sang,
David Woodhouse, Greg Kroah-Hartman, Rob Herring, Mark Brown,
Wolfgang Grandegger, Anatolij Gustschin, linuxppc-dev,
linux-arm-kernel, Mauro Carvalho Chehab
In-Reply-To: <51ED2DF4.2090606@pengutronix.de>
[ adjusted devtree to use the vger address ]
On Mon, Jul 22, 2013 at 15:04 +0200, Marc Kleine-Budde wrote:
>
> On 07/22/2013 02:14 PM, Gerhard Sittig wrote:
> > implement a .get_clock() callback for the MPC512x platform which uses
> > the common clock infrastructure (eliminating direct access to the clock
> > control registers from within the CAN network driver), and provide the
> > corresponding .put_clock() callback to release resources after use
> >
> > keep the previous implementation of MPC512x support in place during
> > migration, since common clock support is optional
> >
> > this change is neutral to the MPC5200 platform
> >
> > Signed-off-by: Gerhard Sittig <gsi@denx.de>
> > ---
> > drivers/net/can/mscan/mpc5xxx_can.c | 169 +++++++++++++++++++++++++++++++++++
> > 1 file changed, 169 insertions(+)
> >
> > diff --git a/drivers/net/can/mscan/mpc5xxx_can.c b/drivers/net/can/mscan/mpc5xxx_can.c
> > index e59b3a3..4897929 100644
> > --- a/drivers/net/can/mscan/mpc5xxx_can.c
> > +++ b/drivers/net/can/mscan/mpc5xxx_can.c
> > @@ -109,6 +109,167 @@ static u32 mpc52xx_can_get_clock(struct platform_device *ofdev,
> > #endif /* CONFIG_PPC_MPC52xx */
> >
> > #ifdef CONFIG_PPC_MPC512x
> > +
> > +#if IS_ENABLED(CONFIG_COMMON_CLK)
> > +
> > +static u32 mpc512x_can_get_clock(struct platform_device *ofdev,
> > + const char *clock_source, int *mscan_clksrc)
> > +{
> > + struct device_node *np;
> > + u32 clockdiv;
> > + enum {
> > + CLK_FROM_AUTO,
> > + CLK_FROM_IPS,
> > + CLK_FROM_SYS,
> > + CLK_FROM_REF,
> > + } clk_from;
> > + struct clk *clk_in, *clk_can;
> > + unsigned long freq_calc;
> > + struct mscan_priv *priv;
> > +
> > + /* the caller passed in the clock source spec that was read from
> > + * the device tree, get the optional clock divider as well
> > + */
> > + np = ofdev->dev.of_node;
> > + clockdiv = 1;
> > + of_property_read_u32(np, "fsl,mscan-clock-divider", &clockdiv);
> > + dev_dbg(&ofdev->dev, "device tree specs: clk src[%s] div[%d]\n",
> > + clock_source ? clock_source : "<NULL>", clockdiv);
> > +
> > + /* when clock-source is 'ip', the CANCTL1[CLKSRC] bit needs to
> > + * get set, and the 'ips' clock is the input to the MSCAN
> > + * component
> > + *
> > + * for clock-source values of 'ref' or 'sys' the CANCTL1[CLKSRC]
> > + * bit needs to get cleared, an optional clock-divider may have
> > + * been specified (the default value is 1), the appropriate
> > + * MSCAN related MCLK is the input to the MSCAN component
> > + *
> > + * in the absence of a clock-source spec, first an optimal clock
> > + * gets determined based on the 'sys' clock, if that fails the
> > + * 'ref' clock is used
> > + */
> > + clk_from = CLK_FROM_AUTO;
> > + if (clock_source) {
> > + /* interpret the device tree's spec for the clock source */
> > + if (!strcmp(clock_source, "ip"))
> > + clk_from = CLK_FROM_IPS;
> > + else if (!strcmp(clock_source, "sys"))
> > + clk_from = CLK_FROM_SYS;
> > + else if (!strcmp(clock_source, "ref"))
> > + clk_from = CLK_FROM_REF;
> > + else
> > + goto err_invalid;
> > + dev_dbg(&ofdev->dev, "got a clk source spec[%d]\n", clk_from);
> > + }
> > + if (clk_from == CLK_FROM_AUTO) {
> > + /* no spec so far, try the 'sys' clock; round to the
> > + * next MHz and see if we can get a multiple of 16MHz
> > + */
> > + dev_dbg(&ofdev->dev, "no clk source spec, trying SYS\n");
> > + clk_in = devm_clk_get(&ofdev->dev, "sys");
> > + if (IS_ERR(clk_in))
> > + goto err_notavail;
> > + freq_calc = clk_get_rate(clk_in);
> > + freq_calc += 499999;
> > + freq_calc /= 1000000;
> > + freq_calc *= 1000000;
> > + if ((freq_calc % 16000000) == 0) {
> > + clk_from = CLK_FROM_SYS;
> > + clockdiv = freq_calc / 16000000;
> > + dev_dbg(&ofdev->dev,
> > + "clk fit, sys[%lu] div[%d] freq[%lu]\n",
> > + freq_calc, clockdiv, freq_calc / clockdiv);
> > + }
> > + }
> > + if (clk_from == CLK_FROM_AUTO) {
> > + /* no spec so far, use the 'ref' clock */
> > + dev_dbg(&ofdev->dev, "no clk source spec, trying REF\n");
> > + clk_in = devm_clk_get(&ofdev->dev, "ref");
> > + if (IS_ERR(clk_in))
> > + goto err_notavail;
> > + clk_from = CLK_FROM_REF;
> > + freq_calc = clk_get_rate(clk_in);
> > + dev_dbg(&ofdev->dev,
> > + "clk fit, ref[%lu] (no div) freq[%lu]\n",
> > + freq_calc, freq_calc);
> > + }
> > +
> > + /* select IPS or MCLK as the MSCAN input (returned to the caller),
> > + * setup the MCLK mux source and rate if applicable, apply the
> > + * optionally specified or derived above divider, and determine
> > + * the actual resulting clock rate to return to the caller
> > + */
> > + switch (clk_from) {
> > + case CLK_FROM_IPS:
> > + clk_can = devm_clk_get(&ofdev->dev, "ips");
> > + if (IS_ERR(clk_can))
> > + goto err_notavail;
> > + if (clk_prepare(clk_can)) {
>
> I would just call prepare_enable in the main mscan driver, then we don't
> need a special "clock is prepared but not enabled" contract.
Yes, I addressed this concern (although differently after
learning more from a local test setup), see the other reply for
the CAN driver adjustment for clock API use.
>
> > + devm_clk_put(&ofdev->dev, clk_can);
>
> not needed, as this driver instance will fail, doesn't it?
This is discussed in the other subthread as well. The only
remaining question is how many of them to remove, before I'll
update the series.
> > + goto err_notavail;
> > + }
> > + priv = netdev_priv(dev_get_drvdata(&ofdev->dev));
> > + priv->clk_can = clk_can;
> > + freq_calc = clk_get_rate(clk_can);
> > + *mscan_clksrc = MSCAN_CLKSRC_IPS;
> > + dev_dbg(&ofdev->dev, "clk from IPS, clksrc[%d] freq[%lu]\n",
> > + *mscan_clksrc, freq_calc);
> > + break;
> > + case CLK_FROM_SYS:
> > + case CLK_FROM_REF:
> > + clk_can = devm_clk_get(&ofdev->dev, "mclk");
> > + if (IS_ERR(clk_can))
> > + goto err_notavail;
> > + if (clk_prepare(clk_can)) {
> > + devm_clk_put(&ofdev->dev, clk_can);
>
> same here
>
> > + goto err_notavail;
> > + }
> > + priv = netdev_priv(dev_get_drvdata(&ofdev->dev));
> > + priv->clk_can = clk_can;
> > + if (clk_from == CLK_FROM_SYS)
> > + clk_in = devm_clk_get(&ofdev->dev, "sys");
> > + if (clk_from == CLK_FROM_REF)
> > + clk_in = devm_clk_get(&ofdev->dev, "ref");
> > + if (IS_ERR(clk_in))
> > + goto err_notavail;
> > + clk_set_parent(clk_can, clk_in);
> > + freq_calc = clk_get_rate(clk_in);
> > + freq_calc /= clockdiv;
> > + clk_set_rate(clk_can, freq_calc);
> > + freq_calc = clk_get_rate(clk_can);
> > + *mscan_clksrc = MSCAN_CLKSRC_BUS;
> > + dev_dbg(&ofdev->dev, "clk from MCLK, clksrc[%d] freq[%lu]\n",
> > + *mscan_clksrc, freq_calc);
> > + break;
> > + default:
> > + goto err_invalid;
> > + }
> > +
> > + return freq_calc;
> > +
> > +err_invalid:
> > + dev_err(&ofdev->dev, "invalid clock source specification\n");
> > + return 0;
>
> return 0 in case of error? Please add a comment what this 0 means here.
The .get_clock() callback is supposed to return the resulting
rate after the clock source was determined and the clock subtree
was setup. It wasn't (explicitly) documented before (in the
non-common-clock case), so I did not bother to comment it in the
parallel common-clock case.
But it's true that returning zero in the error case may be
unexpected, and I will add a comment in v4 (in all the
.get_clock() implementations).
> > +
> > +err_notavail:
> > + dev_err(&ofdev->dev, "cannot acquire or setup clock source\n");
> > + return 0;
> > +}
> > +
> > +static void mpc512x_can_put_clock(struct platform_device *ofdev)
> > +{
> > + struct mscan_priv *priv;
> > +
> > + priv = netdev_priv(dev_get_drvdata(&ofdev->dev));
> > + if (priv->clk_can) {
> > + clk_unprepare(priv->clk_can);
> > + devm_clk_put(&ofdev->dev, priv->clk_can);
>
> devm_clk_put can be removed, it's called automatically.
>
> > + }
> > +}
> > +
> > +#else /* COMMON_CLK */
> > +
> > struct mpc512x_clockctl {
> > u32 spmr; /* System PLL Mode Reg */
> > u32 sccr[2]; /* System Clk Ctrl Reg 1 & 2 */
> > @@ -239,12 +400,18 @@ exit_put:
> > of_node_put(np_clock);
> > return freq;
> > }
> > +
> > +#define mpc512x_can_put_clock NULL
> > +
> > +#endif /* COMMON_CLK */
> > +
> > #else /* !CONFIG_PPC_MPC512x */
> > static u32 mpc512x_can_get_clock(struct platform_device *ofdev,
> > const char *clock_name, int *mscan_clksrc)
> > {
> > return 0;
> > }
> > +#define mpc512x_can_put_clock NULL
> > #endif /* CONFIG_PPC_MPC512x */
> >
> > static const struct of_device_id mpc5xxx_can_table[];
> > @@ -386,11 +553,13 @@ static int mpc5xxx_can_resume(struct platform_device *ofdev)
> > static const struct mpc5xxx_can_data mpc5200_can_data = {
> > .type = MSCAN_TYPE_MPC5200,
> > .get_clock = mpc52xx_can_get_clock,
> > + /* .put_clock not applicable */
> > };
> >
> > static const struct mpc5xxx_can_data mpc5121_can_data = {
> > .type = MSCAN_TYPE_MPC5121,
> > .get_clock = mpc512x_can_get_clock,
> > + .put_clock = mpc512x_can_put_clock,
> > };
> >
> > static const struct of_device_id mpc5xxx_can_table[] = {
> >
>
> Marc
Thank you for the fast and detailled review. It's very much
appreciated.
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: [PATCH v3 01/31] spi: mpc512x: cleanup clock API use
From: Gerhard Sittig @ 2013-07-23 11:58 UTC (permalink / raw)
To: Mark Brown
Cc: devicetree, Mike Turquette, Detlev Zundel, Wolfram Sang,
Greg Kroah-Hartman, linuxppc-dev, Rob Herring, Marc Kleine-Budde,
linux-arm-kernel, Anatolij Gustschin, David Woodhouse,
Wolfgang Grandegger, Mauro Carvalho Chehab
In-Reply-To: <20130722140904.GU9858@sirena.org.uk>
[ devicetree@vger adjusted ]
On Mon, Jul 22, 2013 at 15:09 +0100, Mark Brown wrote:
>
> On Mon, Jul 22, 2013 at 02:14:28PM +0200, Gerhard Sittig wrote:
>
> > + ret = clk_prepare_enable(clk);
> > + if (ret) {
> > + devm_clk_put(dev, clk);
> > + goto free_irq;
>
> The main point of the devm_ APIs is to avoid the need for explicit
> freeing so you should just remove these puts.
OK, will do in v4.
Shall these get removed everywhere including regular shutdown
paths, or just from error paths during setup?
[ the same topic came up for the CAN patch, might answer there ]
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ permalink raw reply
* Re: [PATCH v3 11/31] net: can: mscan: improve clock API use
From: Gerhard Sittig @ 2013-07-23 11:53 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: Mike Turquette, Detlev Zundel, Wolfram Sang, David Woodhouse,
devicetree-discuss, Greg Kroah-Hartman, Rob Herring, Mark Brown,
Wolfgang Grandegger, Anatolij Gustschin, linuxppc-dev,
linux-arm-kernel, Mauro Carvalho Chehab
In-Reply-To: <51ED2619.5040107@pengutronix.de>
On Mon, Jul 22, 2013 at 14:31 +0200, Marc Kleine-Budde wrote:
>
> On 07/22/2013 02:14 PM, Gerhard Sittig wrote:
> > the .get_clock() callback is run from probe() and might allocate
> > resources, introduce a .put_clock() callback that is run from remove()
> > to undo any allocation activities
>
> looks good
>
> > use devm_get_clk() upon lookup (for SYS and REF) to have the clocks put
> > upon driver unload
>
> fine
>
> > assume that resources get prepared but not necessarily enabled in the
> > setup phase, make the open() and close() callbacks of the CAN network
> > device enable and disable a previously acquired and prepared clock
>
> I think you should call prepare_enable and disable_unprepare in the
> open/close functions.
After more local research, which totally eliminated the need to
pre-enable the CAN related clocks, but might need more discussion
as it touches the common gate support, I've learned something
more:
The CAN clock needs to get enabled during probe() already, since
registers get accessed between probe() for the driver and open()
for the network device -- while access to peripheral registers
crashes the kernel when clocks still are disabled (other hardware
may just hang or provide fake data, neither of this is OK).
But I see the point in your suggestion to prepare _and_ enable
the clock during open() as well -- to have open() cope with
whatever probe() did, after all the driver is shared among
platforms, which may differ in what they do during probe().
So I will:
- make open() of the network device prepare _and_ enable the
clock for the peripheral (if acquired during probe())
- adjust open() because ATM it leaves the clock enabled when the
network device operation fails (the error path is incomplete in
v3)
- make the MPC512x specific probe() time .get_clock() routine not
just prepare but enable the clock as well
- and of course address all the shutdown counter parts of the
above setup paths
This results in:
- specific chip drivers only need to balance their private get
and put clock routines which are called from probe and remove,
common paths DTRT for all of them
- correct operation for MPC512x, where common clock is used
- still everything is neutral for MPC5200 where common clock
isn't used, behaviour is identical to before the change
- no assumptions are made about what occurs or doesn't occur
during probe(), when the network device is used then the clock
is fully setup and operational
- when the CAN network device isn't setup (because device tree
doesn't describe it, or disables that node), then its clock
remains idle (neither gets setup nor enabled)
- complete preparation for future improvement wrt power
consumption, where potential changes remain isolated to the
specific chip (probe() time setup, get_clock() routine) while
the ndo part need not get touched any more
So this is the most appropriate approach I can come up with.
Removing unnecessary devm_put_clk() calls is orthogonal to that.
Putting these in isn't totally wrong (they won't harm, and they
do signal "visual balance" more clearly such that the next person
won't stop and wonder), but it's true that they are redundant.
"Trained persons" will wonder as much about their presence as
untrained persons wonder about their absence. :) Apparently I'm
not well trained yet.
I thought that being explicit and cautious would be good, but the
feedback I got suggests that encoding unnecessary instructions
isn't desirable. So I will remove those devm_put_clk() in v4.
To save us one more iteration, shall I remove those calls only
from error paths during setup? Or shall I remove them from
regular shutdown paths as well? How much pain does the community
feel with harmless yet unnecessary instructions? :)
virtually yours
Gerhard Sittig
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office@denx.de
^ 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