Re: [PATCH] Kconfig: Remove hotplug enable hints in CONFIG_KEXEC help texts
* Geert Uytterhoeven wrote:
> commit 40b313608ad4ea655addd2ec6cdd106477ae8e15 ("Finally eradicate
> CONFIG_HOTPLUG") removed remaining references to CONFIG_HOTPLUG, but missed
> a few plain English references in the CONFIG_KEXEC help texts.
>
> Remove them, too.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> arch/arm/Kconfig |3 +--
> arch/ia64/Kconfig|6 +++---
> arch/mips/Kconfig|6 +++---
> arch/powerpc/Kconfig |6 +++---
> arch/sh/Kconfig |6 +++---
> arch/x86/Kconfig |6 +++---
> 6 files changed, 16 insertions(+), 17 deletions(-)
[...]
> bool "kernel crash dumps (EXPERIMENTAL)"
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index b32ebf9..6ace5de 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1627,9 +1627,9 @@ config KEXEC
>
> It is an ongoing process to be certain the hardware in a machine
> is properly shutdown, so do not be surprised if this code does not
> - initially work for you. It may help to enable device hotplugging
> - support. As of this writing the exact hardware interface is
> - strongly in flux, so no good recommendation can be made.
> + initially work for you. As of this writing the exact hardware
> + interface is strongly in flux, so no good recommendation can be
> + made.
>
> config CRASH_DUMP
> bool "kernel crash dumps"
Acked-by: Ingo Molnar
Thanks,
Ingo
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 2/8] net: ucc_geth: remove unnecessary dev_set_drvdata()
Unnecessary dev_set_drvdata() is removed, because the driver core clears the driver data to NULL after device_release or on probe failure. Signed-off-by: Libo Chen --- drivers/net/ethernet/freescale/ucc_geth.c |1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c index 533885c..5930c39 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.c +++ b/drivers/net/ethernet/freescale/ucc_geth.c @@ -3917,7 +3917,6 @@ static int ucc_geth_remove(struct platform_device* ofdev) unregister_netdev(dev); free_netdev(dev); ucc_geth_memclean(ugeth); - dev_set_drvdata(device, NULL); return 0; } -- 1.7.1 ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH 0/8] net: remove unnecessary dev_set_drvdata()
Unnecessary dev_set_drvdata() is removed, because the driver core clears the driver data to NULL after device_release or on probe failure. Libo Chen (8): net: fsl_pq_mdio: remove unnecessary dev_set_drvdata() net: ucc_geth: remove unnecessary dev_set_drvdata() net: fec_mpc52xx_phy: remove unnecessary dev_set_drvdata() net: sunbmac: remove unnecessary dev_set_drvdata() net: sunhme: remove unnecessary dev_set_drvdata() net: xilinx_emaclite: remove unnecessary dev_set_drvdata() net: davinci_mdio: remove unnecessary dev_set_drvdata() net: emac: remove unnecessary dev_set_drvdata() drivers/net/ethernet/freescale/fec_mpc52xx_phy.c |1 - drivers/net/ethernet/freescale/fsl_pq_mdio.c |2 -- drivers/net/ethernet/freescale/ucc_geth.c|1 - drivers/net/ethernet/ibm/emac/core.c |2 -- drivers/net/ethernet/sun/sunbmac.c |2 -- drivers/net/ethernet/sun/sunhme.c|2 -- drivers/net/ethernet/ti/davinci_mdio.c |2 -- drivers/net/ethernet/xilinx/xilinx_emaclite.c|1 - 8 files changed, 0 insertions(+), 13 deletions(-) ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote: > Quoting Mark Rutland (2013-08-19 02:35:43) > > On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote: > > > On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: > > > > On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: > > > > > > > > Also I would make this option required. Use a dummy clock for > > > > > > > > mux > > > > > > > > inputs that are grounded for a specific SoC. > > > > > > > > > > > > > > Some clocks are not from CCM and we haven't defined in > > > > > > > imx6q-clk.txt, > > > > > > > so in most cases we can't provide a phandle for them, eg: > > > > > > > spdif_ext. > > > > > > > I think it's a bit hard to force it to be 'required'. An > > > > > > > 'optional' > > > > > > > looks more flexible to me and a default one is ensured even if > > > > > > > it's > > > > > > > missing. > > > > > > > > > > > > <&clks 0> is the dummy clock. This can be used for all input clocks > > > > > > not > > > > > > defined by the SoC. > > > > > > > > > > Where does this assumption come from? Is it documented anywhere? > > > > > > > > This is how all i.MX clock bindings currently are. See > > > > Documentation/devicetree/bindings/clock/imx*-clock.txt > > > > > > OK, thanks. > > > > > > I guess we need some discussion on dummy clocks vs skipped clocks. I > > > think > > > we want some consistency on this, don't we? > > > > > > If we really need a dummy clock, then we might also want a generic way to > > > specify it. > > > > What do we actually mean by a "dummy clock"? We already have bindings > > for "fixed-clock" and co friends describe relatively simple > > preconfigured clocks. > > Some platforms have a fake clock which defines noops callbacks and > basically doesn't do anything. This is analogous to the dummy regulator > implementation. A central one could be registered by the clock core, as > is done by the regulator core. When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT: * If a clock is wired up and running (as presumably the dummy clock is), then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such? * If no clock is wired up, then we should be able to describe that. If a driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional. Am I missing something? Thanks, Mark. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] Device Tree bindings for DSP clusters and DSP CPUs
Binding for DSP CPU clusters and DSP CPUs for Freescale SOCs which
have DSP CPUs in addition to PowerPC CPUs. For example B4860.
Signed-off-by: Poonam Aggrwal
---
.../devicetree/bindings/powerpc/fsl/dsp-cpus.txt | 78
1 files changed, 78 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/powerpc/fsl/dsp-cpus.txt
diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dsp-cpus.txt
b/Documentation/devicetree/bindings/powerpc/fsl/dsp-cpus.txt
new file mode 100644
index 000..da7f5d4
--- /dev/null
+++ b/Documentation/devicetree/bindings/powerpc/fsl/dsp-cpus.txt
@@ -0,0 +1,78 @@
+===
+Binding for DSP CPU clusters and DSP CPUs for Freescale SOCs which
+have DSP CPUs in addition to PowerPC cpus.
+Copyright 2013 Freescale Semiconductor Inc.
+
+Power Architecture CPUs in Freescale SOCs are represented in device trees as
+per the definition in ePAPR.
+
+Required properties for DSP CPU cluster:
+- compatible : should be "fsl,dsp-cluster" or "fsl,sc3900-cluster".
+- reg : should contain the cluster index
+
+Required properties for DSP CPU:
+- compatible : should be "fsl,dsp" or "fsl,sc3900".
+- reg : should contain index of DSP CPU within the DSP clsuter.
+- next-level-cache : should point to the phandle of the next-level L2 cache.
+
+Example for B4860:
+B4860 SOC of Freescale has 3 DSP clusters. Each DSP cluster has 2 DSP CPUs
each.
+The DSP CPUs are SC3900. There is a shared L2 cache per DSP cluster.
+ dsp-clusters {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ dsp-cluster0 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,sc3900-cluster";
+ reg = <0>;
+
+ dsp0: dsp@0 {
+ compatible = "fsl,sc3900";
+ reg = <0>;
+ next-level-cache = <&L2_2>;
+ };
+ dsp1: dsp@1 {
+ compatible = "fsl,sc3900";
+ reg = <1>;
+ next-level-cache = <&L2_2>;
+ };
+ };
+
+ dsp-cluster1 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,sc3900-cluster";
+ reg = <1>;
+
+ dsp2: dsp@2 {
+ compatible = "fsl,sc3900";
+ reg = <2>;
+ next-level-cache = <&L2_3>;
+ };
+ dsp3: dsp@3 {
+ compatible = "fsl,sc3900";
+ reg = <3>;
+ next-level-cache = <&L2_3>;
+ };
+ };
+
+ dsp-cluster2 {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ compatible = "fsl,sc3900-cluster";
+ reg = <2>;
+
+ dsp4: dsp@4 {
+ compatible = "fsl,sc3900";
+ reg = <4>;
+ next-level-cache = <&L2_4>;
+ };
+ dsp5: dsp@5 {
+ compatible = "fsl,sc3900";
+ reg = <5>;
+ next-level-cache = <&L2_4>;
+ };
+ };
+ };
--
1.7.4.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc: add the missing required isync for the coherent icache flush
On Wed, Aug 21, 2013 at 07:28:54AM +1000, Benjamin Herrenschmidt wrote: > On Tue, 2013-08-20 at 20:16 +0800, Kevin Hao wrote: > > > Dummy question: What does the ifetch buffers mean? The instruction fetch > > pipeline or instruction dispatch pipeline? Shouldn't all the prefetched > > instructions in these buffers be discarded by isync? > > Architecturally isync doesn't have to toss prefetch completely. It only > needs to make sense that context changes performed by previous > instructions (and interrupts/traps) happen at the point of the isync, > for example, it will ensure that a trap conditional is fully evaluated > before subsequent instruction execution, etc > > So in this case, it makes sure the icbi has been executed and it's the > icbi that invalidates the prefetched instructions. > > > > > >, sync orders the icbi and isync ensures its execution has been > > > synchronized. At least I *think* that's the required sequence, I have to > > > dbl check the arch, maybe tomorrow. I wouldn't be surprise if we also > > > need a sync before the icbi to order the actual stores to memory that > > > might have modified instructions with the icbi. > > > > Doesn't the coherence between icache and dcache be maintained by the > > snooping? > > The icache yes, but not the ifetch buffers (basically think of them as > buffering stages between icache and dispatch). At least that's my > understanding of the implementation. OK. Thanks for the explanation. I will update the patch according to your suggestions. Thanks, Kevin pgp8ceQ23eRVX.pgp Description: PGP signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] crypto:nx - fix nx-aes-gcm verification
On Wed, Aug 14, 2013 at 05:17:57PM -0500, jmlat...@linux.vnet.ibm.com wrote: > This patch fixes a bug in the nx-aes-gcm implementation. > Corrected the code so that the authtag is always verified after > decrypting and not just when there is associated data included. > Also, corrected the code to retrieve the input authtag from src > instead of dst. > > Reviewed-by: Fionnuala Gunter > Reviewed-by: Marcelo Cerri > Signed-off-by: Joy Latten Patch applied. Thanks! -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V2] i2c: move of helpers into the core
I2C of helpers used to live in of_i2c.c but experience (from SPI) shows that it is much cleaner to have this in the core. This also removes a circular dependency between the helpers and the core, and so we can finally register child nodes in the core instead of doing this manually in each driver. So, fix the drivers and documentation, too. Acked-by: Sylwester Nawrocki Acked-by: Rob Herring Reviewed-by: Felipe Balbi Acked-by: Rafael J. Wysocki Signed-off-by: Wolfram Sang --- V1 -> V2: * Add #else branch to #if CONFIG_OF * EXPORT_SYMBOLs got attached to wrong functions * cosmetic change (of -> OF) * properly based on 3.11-rc4 Documentation/acpi/enumeration.txt |1 - drivers/i2c/busses/i2c-at91.c |3 - drivers/i2c/busses/i2c-cpm.c|6 -- drivers/i2c/busses/i2c-davinci.c|2 - drivers/i2c/busses/i2c-designware-platdrv.c |2 - drivers/i2c/busses/i2c-gpio.c |3 - drivers/i2c/busses/i2c-i801.c |2 - drivers/i2c/busses/i2c-ibm_iic.c|4 - drivers/i2c/busses/i2c-imx.c|3 - drivers/i2c/busses/i2c-mpc.c|2 - drivers/i2c/busses/i2c-mv64xxx.c|3 - drivers/i2c/busses/i2c-mxs.c|3 - drivers/i2c/busses/i2c-nomadik.c|3 - drivers/i2c/busses/i2c-ocores.c |3 - drivers/i2c/busses/i2c-octeon.c |3 - drivers/i2c/busses/i2c-omap.c |3 - drivers/i2c/busses/i2c-pnx.c|3 - drivers/i2c/busses/i2c-powermac.c |9 +- drivers/i2c/busses/i2c-pxa.c|2 - drivers/i2c/busses/i2c-s3c2410.c|2 - drivers/i2c/busses/i2c-sh_mobile.c |2 - drivers/i2c/busses/i2c-sirf.c |3 - drivers/i2c/busses/i2c-stu300.c |2 - drivers/i2c/busses/i2c-tegra.c |3 - drivers/i2c/busses/i2c-versatile.c |2 - drivers/i2c/busses/i2c-wmt.c|3 - drivers/i2c/busses/i2c-xiic.c |3 - drivers/i2c/i2c-core.c | 109 +- drivers/i2c/i2c-mux.c |3 - drivers/i2c/muxes/i2c-arb-gpio-challenge.c |1 - drivers/i2c/muxes/i2c-mux-gpio.c|1 - drivers/i2c/muxes/i2c-mux-pinctrl.c |1 - drivers/media/platform/exynos4-is/fimc-is-i2c.c |3 - drivers/of/Kconfig |6 -- drivers/of/Makefile |1 - drivers/of/of_i2c.c | 114 --- include/linux/i2c.h | 20 include/linux/of_i2c.h | 46 - 38 files changed, 132 insertions(+), 253 deletions(-) delete mode 100644 drivers/of/of_i2c.c delete mode 100644 include/linux/of_i2c.h diff --git a/Documentation/acpi/enumeration.txt b/Documentation/acpi/enumeration.txt index d9be7a9..958266e 100644 --- a/Documentation/acpi/enumeration.txt +++ b/Documentation/acpi/enumeration.txt @@ -238,7 +238,6 @@ An I2C bus (controller) driver does: if (ret) /* handle error */ - of_i2c_register_devices(adapter); /* Enumerate the slave devices behind this bus via ACPI */ acpi_i2c_register_devices(adapter); diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c index 6bb839b..fd05930 100644 --- a/drivers/i2c/busses/i2c-at91.c +++ b/drivers/i2c/busses/i2c-at91.c @@ -28,7 +28,6 @@ #include #include #include -#include #include #include #include @@ -775,8 +774,6 @@ static int at91_twi_probe(struct platform_device *pdev) return rc; } - of_i2c_register_devices(&dev->adapter); - dev_info(dev->dev, "AT91 i2c bus driver.\n"); return 0; } diff --git a/drivers/i2c/busses/i2c-cpm.c b/drivers/i2c/busses/i2c-cpm.c index 2e1f7eb..b2b8aa9 100644 --- a/drivers/i2c/busses/i2c-cpm.c +++ b/drivers/i2c/busses/i2c-cpm.c @@ -42,7 +42,6 @@ #include #include #include -#include #include #include @@ -681,11 +680,6 @@ static int cpm_i2c_probe(struct platform_device *ofdev) dev_dbg(&ofdev->dev, "hw routines for %s registered.\n", cpm->adap.name); - /* -* register OF I2C devices -*/ - of_i2c_register_devices(&cpm->adap); - return 0; out_shut: cpm_i2c_shutdown(cpm); diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c index fa55605..62be3b3 100644 --- a/drivers/i2c/busses/i2c-davinci.c +++ b/drivers/i2c/busses/i2c-davinci.c @@ -38,7 +38,6 @@ #include #include #include -#include #include #include @@ -728,7 +727,6 @@ static int davinci_i2c_probe(struct platform_device *pdev
Re: [PATCH V2] i2c: move of helpers into the core
On 08/21/2013 03:47 PM, Wolfram Sang wrote: > I2C of helpers used to live in of_i2c.c but experience (from SPI) shows > that it is much cleaner to have this in the core. This also removes a > circular dependency between the helpers and the core, and so we can > finally register child nodes in the core instead of doing this manually > in each driver. So, fix the drivers and documentation, too. > > Acked-by: Sylwester Nawrocki > Acked-by: Rob Herring > Reviewed-by: Felipe Balbi > Acked-by: Rafael J. Wysocki > Signed-off-by: Wolfram Sang With this patch there are still couple of of_i2c.h header file inclusions: $ git grep of_i2c.h arch/powerpc/platforms/44x/warp.c:#include drivers/gpu/drm/tilcdc/tilcdc_slave.c:#include drivers/gpu/drm/tilcdc/tilcdc_tfp410.c:#include drivers/gpu/host1x/drm/output.c:#include drivers/media/platform/exynos4-is/fimc-is.c:#include drivers/media/platform/exynos4-is/media-dev.c:#include drivers/staging/imx-drm/imx-tve.c:#include sound/soc/fsl/imx-sgtl5000.c:#include sound/soc/fsl/imx-wm8962.c:#include Please include also this chunk, without it I'm getting build errors. --8<- diff --git a/drivers/media/platform/exynos4-is/fimc-is-i2c.c b/drivers/media/platform/exynos4-is/fimc-is-i2c.c index ca07b48..e38e9dc 100644 --- a/drivers/media/platform/exynos4-is/fimc-is-i2c.c +++ b/drivers/media/platform/exynos4-is/fimc-is-i2c.c @@ -11,6 +11,7 @@ */ #include +#include #include #include #include diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c index 6743ae3..63e4f1d 100644 --- a/drivers/media/platform/exynos4-is/fimc-is.c +++ b/drivers/media/platform/exynos4-is/fimc-is.c @@ -21,7 +21,6 @@ #include #include #include -#include #include #include #include diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c index c10dee2..00e5f91 100644 --- a/drivers/media/platform/exynos4-is/media-dev.c +++ b/drivers/media/platform/exynos4-is/media-dev.c @@ -22,7 +22,6 @@ #include #include #include -#include #include #include #include --8<- > --- > > V1 -> V2: * Add #else branch to #if CONFIG_OF > * EXPORT_SYMBOLs got attached to wrong functions > * cosmetic change (of -> OF) > * properly based on 3.11-rc4 > > Documentation/acpi/enumeration.txt |1 - > drivers/i2c/busses/i2c-at91.c |3 - > drivers/i2c/busses/i2c-cpm.c|6 -- > drivers/i2c/busses/i2c-davinci.c|2 - > drivers/i2c/busses/i2c-designware-platdrv.c |2 - > drivers/i2c/busses/i2c-gpio.c |3 - > drivers/i2c/busses/i2c-i801.c |2 - > drivers/i2c/busses/i2c-ibm_iic.c|4 - > drivers/i2c/busses/i2c-imx.c|3 - > drivers/i2c/busses/i2c-mpc.c|2 - > drivers/i2c/busses/i2c-mv64xxx.c|3 - > drivers/i2c/busses/i2c-mxs.c|3 - > drivers/i2c/busses/i2c-nomadik.c|3 - > drivers/i2c/busses/i2c-ocores.c |3 - > drivers/i2c/busses/i2c-octeon.c |3 - > drivers/i2c/busses/i2c-omap.c |3 - > drivers/i2c/busses/i2c-pnx.c|3 - > drivers/i2c/busses/i2c-powermac.c |9 +- > drivers/i2c/busses/i2c-pxa.c|2 - > drivers/i2c/busses/i2c-s3c2410.c|2 - > drivers/i2c/busses/i2c-sh_mobile.c |2 - > drivers/i2c/busses/i2c-sirf.c |3 - > drivers/i2c/busses/i2c-stu300.c |2 - > drivers/i2c/busses/i2c-tegra.c |3 - > drivers/i2c/busses/i2c-versatile.c |2 - > drivers/i2c/busses/i2c-wmt.c|3 - > drivers/i2c/busses/i2c-xiic.c |3 - > drivers/i2c/i2c-core.c | 109 +- > drivers/i2c/i2c-mux.c |3 - > drivers/i2c/muxes/i2c-arb-gpio-challenge.c |1 - > drivers/i2c/muxes/i2c-mux-gpio.c|1 - > drivers/i2c/muxes/i2c-mux-pinctrl.c |1 - > drivers/media/platform/exynos4-is/fimc-is-i2c.c |3 - > drivers/of/Kconfig |6 -- > drivers/of/Makefile |1 - > drivers/of/of_i2c.c | 114 > --- > include/linux/i2c.h | 20 > include/linux/of_i2c.h | 46 - > 38 files changed, 132 insertions(+), 253 deletions(-) > delete mode 100644 drivers/of/of_i2c.c > delete mode 100644 include/linux/of_i2c.h I've tested this patch on Exynos4412 SoC based board, so this covers i2c-s3c2410 and fimc-is-i2c. Compiled with CONFIG_
Re: linux-next: build failure after merge of the final tree
Hey Stephen, On Wed, Aug 21, 2013 at 10:22:46AM +1000, Stephen Rothwell wrote: > On Tue, 20 Aug 2013 14:28:44 -0500 Ben Myers wrote: > > I'd prefer not to break Stephen's tree two days in a row. We could just > > revert > > d6970d4b726c in the xfs tree for the time being as Stephen has done, but > > given > > the choice would prefer the fix. Do you have a preference between the two > > approaches that Dwight has posted? The first seems more conservative... > > I will automatically revert that commit when I merge the xfs tree until > some other solution is forthcoming (so you don't have to do the revert in > the xfs tree). Gah. That makes sense. ;) > This does need to be fixed fairly soon, though. Agreed, thanks. -Ben ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] Adding proper request of GPIO used by cpm_uart driver
cpm_uart serial driver uses GPIO for control signals. In order to be used
properly, GPIOs have to be reserved. Comment in gpiolib.c considers illegal
the use of GPIOs without requesting them. In addition, the direction of the
GPIO has to be set properly.
Signed-off-by: Christophe Leroy
diff -ur linux-3.8.13/drivers/tty/serial/cpm_uart/cpm_uart_core.c
linux/drivers/tty/serial/cpm_uart/cpm_uart_core.c
--- linux-3.8.13/drivers/tty/serial/cpm_uart/cpm_uart_core.c2013-08-21
05:34:03.0 +0200
+++ linux/drivers/tty/serial/cpm_uart/cpm_uart_core.c 2013-08-21
05:30:04.0 +0200
@@ -1213,8 +1213,32 @@
goto out_pram;
}
- for (i = 0; i < NUM_GPIOS; i++)
- pinfo->gpios[i] = of_get_gpio(np, i);
+ for (i = 0; i < NUM_GPIOS; i++) {
+ int gpio;
+
+ pinfo->gpios[i] = -1;
+
+ gpio = of_get_gpio(np, i);
+
+ if (gpio_is_valid(gpio)) {
+ ret = gpio_request(gpio, "cpm_uart");
+ if (ret) {
+ pr_err("can't request gpio #%d: %d\n", i, ret);
+ continue;
+ }
+ if (i == GPIO_RTS || i == GPIO_DTR)
+ ret = gpio_direction_output(gpio, 0);
+ else
+ ret = gpio_direction_input(gpio);
+ if (ret) {
+ pr_err("can't set direction for gpio #%d: %d\n",
+ i, ret);
+ gpio_free(gpio);
+ continue;
+ }
+ pinfo->gpios[i] = gpio;
+ }
+ }
#ifdef CONFIG_PPC_EARLY_DEBUG_CPM
udbg_putc = NULL;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: linux-next: build failure after merge of the final tree
Hey Dwight, On Wed, Aug 21, 2013 at 02:30:04PM +0800, Jeremy Kerr wrote: > > Yes, I agree. The other filesystems that take an Opt_uid as well do use > > current_user_ns() and not init_user_ns. They also do a uid_valid() > > check and fail the mount (or fallback to GLOBAL_ROOT_UID). So I think > > that would look like this: > > Looks good to me. Builds and mounts as expected. > > Acked-by: Jeremy Kerr Could you repost this patch with the right subject and a commit header? Given Jeremy's Ack I think we could proceed to pull this in. Regards, Ben ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [alsa-devel] [PATCH v8 2/2] ASoC: fsl: Add S/PDIF machine driver
On 08/20/2013 08:18 PM, Nicolin Chen wrote: > On Tue, Aug 20, 2013 at 11:28:10PM +0100, Mark Brown wrote: >> On Tue, Aug 20, 2013 at 01:53:49PM -0600, Stephen Warren wrote: >>> On 08/20/2013 01:07 PM, Mark Brown wrote: >> The point is that it might turn into a more correct binding depending on what the S/PDIF device actually is. >> >>> There's *never* an object on the board called a "dummy codec". >> >> Oh, is that what you're talking about? Yes, that makes sense. I had >> been responding to the comments about the transceivers. > > I'll remove the 'dummy' words in the next version from the binding doc. I think the word "CODEC" is also problematic in this context, since whatever is connector to the S/PDIF output path may not be a CODEC. That's why I suggested some more generic property names that IIRC concentrated on enabling rx/tx rather than indicating what was actually connected to the S/PDIF controller. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v10 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On 08/20/2013 09:13 PM, Nicolin Chen wrote: > This patch implements a device-tree-only CPU DAI driver for Freescale > S/PDIF controller that supports stereo playback and record feature. The DT bindings part of this patch, Acked-by: Stephen Warren ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v10 2/2] ASoC: fsl: Add S/PDIF machine driver
On 08/20/2013 09:13 PM, Nicolin Chen wrote: > This patch implements a device-tree-only machine driver for Freescale > i.MX series Soc. It works with spdif_transmitter/spdif_receiver and > fsl_spdif.c drivers. > diff --git a/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt > b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt > +Optional properties: > + > + - spdif-transmitter : The phandle of the spdif-transmitter codec > + > + - spdif-receiver : The phandle of the spdif-receiver codec > + > +* Note: At least one of these two properties should be set in the DT binding. I still don't think those two properties are correct. Exactly what node will those phandles point at? There definitely should not be a DT node for any "dummy CODEC", irrespective of whether this binding calls the other node a "CODEC" or a "dummy CODEC". If these properties are to contain phandles, it would be acceptable for the referenced node to be: * A node representing the physical connector/jack on the board. * A node representing some other IP block on the board, such as an HDMI encoder/display-controller I think those options are unlikely in general, so I think instead these properties should just be Boolean indicating that "something" is connector to the S/PDIF RX/TX, without specifying what that "something" is. It doesn't matter what at least in the connector/jack case, although perhaps it does in the HDMI encoder/display-controller? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc/spufs: convert userns uid/gid mount options to kuid/kgid
Acked-by: Jeremy Kerr Tested-by: Jeremy Kerr Signed-off-by: Dwight Engen --- arch/powerpc/platforms/cell/spufs/inode.c | 8 ++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/arch/powerpc/platforms/cell/spufs/inode.c b/arch/powerpc/platforms/cell/spufs/inode.c index f390042..87ba7cf 100644 --- a/arch/powerpc/platforms/cell/spufs/inode.c +++ b/arch/powerpc/platforms/cell/spufs/inode.c @@ -620,12 +620,16 @@ spufs_parse_options(struct super_block *sb, char *options, struct inode *root) case Opt_uid: if (match_int(&args[0], &option)) return 0; - root->i_uid = option; + root->i_uid = make_kuid(current_user_ns(), option); + if (!uid_valid(root->i_uid)) + return 0; break; case Opt_gid: if (match_int(&args[0], &option)) return 0; - root->i_gid = option; + root->i_gid = make_kgid(current_user_ns(), option); + if (!gid_valid(root->i_gid)) + return 0; break; case Opt_mode: if (match_octal(&args[0], &option)) -- 1.8.1.4 On Wed, 21 Aug 2013 10:56:54 -0500 Ben Myers wrote: > Hey Dwight, > > On Wed, Aug 21, 2013 at 02:30:04PM +0800, Jeremy Kerr wrote: > > > Yes, I agree. The other filesystems that take an Opt_uid as well > > > do use current_user_ns() and not init_user_ns. They also do a > > > uid_valid() check and fail the mount (or fallback to > > > GLOBAL_ROOT_UID). So I think that would look like this: > > > > Looks good to me. Builds and mounts as expected. > > > > Acked-by: Jeremy Kerr > > Could you repost this patch with the right subject and a commit > header? Given Jeremy's Ack I think we could proceed to pull this in. Sure, I just wanted to make sure someone had tested it first, which it looks like Jeremy did, thanks. > Regards, > Ben ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 1/7] drivers: base: move mutex lock out of add_memory_section()
On Tue, Aug 20, 2013 at 12:24:45PM -0500, Seth Jennings wrote: > Gah! Forgot the cover letter. No worries, I barely read them anyway :) > This patchset just seeks to clean up and refactor some things in > memory.c for better understanding and possibly better performance due do > a decrease in mutex acquisitions and refcount churn at boot time. No > functional change is intended by this set! All looks good, thanks for breaking it up into reviewable patches. Now applied. greg k-h ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v10 2/2] ASoC: fsl: Add S/PDIF machine driver
On Wednesday 21 of August 2013 12:30:59 Stephen Warren wrote:
> On 08/20/2013 09:13 PM, Nicolin Chen wrote:
> > This patch implements a device-tree-only machine driver for Freescale
> > i.MX series Soc. It works with spdif_transmitter/spdif_receiver and
> > fsl_spdif.c drivers.
> >
> > diff --git
> > a/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
> > b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
> >
> > +Optional properties:
> > +
> > + - spdif-transmitter : The phandle of the spdif-transmitter codec
> > +
> > + - spdif-receiver : The phandle of the spdif-receiver codec
> > +
> > +* Note: At least one of these two properties should be set in the DT
> > binding.
> I still don't think those two properties are correct.
>
> Exactly what node will those phandles point at?
Imagine following setup:
|| RX | Microphone DSP | Analog mic input
| S/PDIF | << || <---
||
| DAI | >> | Amplifier | >---
|| TX || Speakers output
As you see in the diagram, the S/PDIF interface of the SoC can be
connected to some external devices that can perform sound processing or
simply handle the physical layer.
I'd say that normally both RX and TX lines would be connected to a single
codec chip that has multiple blocks inside, like sound processing,
amplifier, mixer, etc., but nothing stops you from making a crazy setup,
when RX and TX lines are connected to different chips.
> There definitely should not be a DT node for any "dummy CODEC",
> irrespective of whether this binding calls the other node a "CODEC" or a
> "dummy CODEC".
I agree. Instead if no chip connected to particular line is specified in
device tree, it's responsibility of Linux sound core to handle this
properly by adding a dummy codec or whatever.
> If these properties are to contain phandles, it would be acceptable for
> the referenced node to be:
>
> * A node representing the physical connector/jack on the board.
>
> * A node representing some other IP block on the board, such as an HDMI
> encoder/display-controller
>
> I think those options are unlikely in general
Why? You usually codec SoC DAIs to some external chips.
Best regards,
Tomasz
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 01/31] spi: mpc512x: cleanup clock API use
On Tue, 6 Aug 2013 22:43:41 +0200 Gerhard Sittig wrote: > cleanup the MPC512x SoC's SPI master's use of the clock API > - get, prepare, and enable the MCLK during probe; disable, unprepare and > put the MCLK upon remove; hold a reference to the clock over the > period of use > - fetch MCLK rate (reference) once during probe and slightly reword BCLK > (bitrate) determination to reduce redundancy as well as to not exceed > the maximum text line length > - stick with the PPC_CLOCK 'psc%d_mclk' name for clock lookup, only > switch to a fixed string later after device tree based clock lookup > will have become available > > Signed-off-by: Gerhard Sittig > --- > drivers/spi/spi-mpc512x-psc.c | 48 > + > 1 file changed, 30 insertions(+), 18 deletions(-) Mark, are you going to apply this patch? Or should I queue it in my mpc5xxx tree (I'd like to get your Acked-by then)? Thanks, Anatolij ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 0/8] net: remove unnecessary dev_set_drvdata()
From: Libo Chen Date: Wed, 21 Aug 2013 15:05:37 +0800 > Unnecessary dev_set_drvdata() is removed, because the driver core > clears the driver data to NULL after device_release or on probe failure. Series applied, thanks. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 01/31] spi: mpc512x: cleanup clock API use
On Wed, Aug 21, 2013 at 09:22:58PM +0200, Anatolij Gustschin wrote: > Mark, are you going to apply this patch? Or should I queue it > in my mpc5xxx tree (I'd like to get your Acked-by then)? Has this series settled down? I'd been ignoring it since it was getting so many and so frequent revisions. signature.asc Description: Digital signature ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 02/31] serial: mpc512x: cleanup clock API use
On Tue, 6 Aug 2013 22:43:42 +0200 Gerhard Sittig wrote: > cleanup the clock API use of the UART driver which is shared among the > MPC512x and the MPC5200 platforms > - get, prepare, and enable the MCLK during port allocation; disable, > unprepare and put the MCLK upon port release; hold a reference to the > clock over the period of use; check for and propagate enable errors > - fix a buffer overflow for clock names with two digit PSC index numbers > - stick with the PPC_CLOCK 'psc%d_mclk' name for clock lookup, only > switch to a fixed string later after device tree based clock lookup > will have become available > > to achieve support for MPC512x which is neutral to MPC5200, the > modification was done as follows > - introduce "clock alloc" and "clock release" routines in addition to > the previous "clock enable/disable" routine in the psc_ops struct > - make the clock allocation a part of the port request (resource > allocation), and make clock release a part of the port release, such > that essential resources get allocated early > - just enable/disable the clock from within the .clock() callback > without any allocation or preparation as the former implementation > did, since this routine is called from within the startup and shutdown > callbacks > - all of the above remains a NOP for the MPC5200 platform (no callbacks > are provided on that platform) > - implementation note: the clock gets enabled upon allocation already > just in case the clock is not only required for bitrate generation but > for register access as well > > Signed-off-by: Gerhard Sittig > --- > drivers/tty/serial/mpc52xx_uart.c | 98 > ++--- > 1 file changed, 81 insertions(+), 17 deletions(-) applied, thanks! Anatolij ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/spufs: convert userns uid/gid mount options to kuid/kgid
On Wednesday 21 August 2013, Dwight Engen wrote: > > Acked-by: Jeremy Kerr > Tested-by: Jeremy Kerr > Signed-off-by: Dwight Engen Reviewed-by: Arnd Bergmann ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH V3 3/5] powerpc/cpuidle: Generic powerpc backend cpuidle driver.
On Wed, 2013-08-21 at 10:23 +0530, Deepthi Dharwar wrote: > On 08/19/2013 11:47 PM, Scott Wood wrote: > > On Mon, 2013-08-19 at 15:48 +0530, Deepthi Dharwar wrote: > >> Hi Dongsheng, > >> > >> On 08/19/2013 11:22 AM, Wang Dongsheng-B40534 wrote: > >>> I think we should move the states and handle function to > >>> arch/power/platform* > >>> The states and handle function is belong to backend driver, not for this, > >>> different platform have different state. > >>> Different platforms to make their own deal with these states. > >>> > >>> I think we cannot put all the status of different platforms and handler > >>> in this driver. > >> > >> The idea here is a single powerpc back-end driver, which does a runtime > >> detection of the platform it is running and choose the right > >> idle states table. This was one of outcome of V2 discussion. > > > > I see a lot more in there than just detecting a platform and choosing a > > driver. > > > >> I feel there is no harm in keeping the state information in the same > >> file. We do have x86, which has all its variants information in one > >> file. One place will have all the idle consolidated information of > >> all the platform variants. If community does feel, we need to > >> have just the states information in arch specific file, we can do so. > > > > What actual functionality is common to all powerpc but not common to > > other arches? No answer? > +config CPU_IDLE_POWERPC > +bool "CPU Idle driver for POWERPC platforms" > +depends on PPC64 > >>> > >>> Why not PPC? > >> > >> PPC64 seems to a good place to began the consolidation work. This > >> patch-set has not been tested for PPC32 currently. > > > > PPC64 is a bad place to start if you want it to be generic, because it > > means you'll end up growing dependencies on other things that are PPC64 > > only. There are too many arbitrary 32/64 differences as is. > > Hi Scott, > > From my understanding, PPC64 includes BOOK3E and BOOK3S archs. > PPC includes PPC32 and PPC64. > > It seemed logical to start consolidating at PPC64 as > one does not want to get into 32/64 bit differences. I don't want to "get into" a file that claims to be generic PPC but is loaded with 64-bit dependencies. > From your comments above, I just wanted to clarify if PPC or PPC64 is > bad place to start. If PPC64 is bad place to start, then whats the way > forward ? Can you please throw some more light on it. The way forward is to give this file a more appropriate name based on the hardware that it actually targets -- and to refactor it so that the answer to that question is not complicated. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/spufs: convert userns uid/gid mount options to kuid/kgid
On Wed, Aug 21, 2013 at 10:05:27PM +0200, Arnd Bergmann wrote: > On Wednesday 21 August 2013, Dwight Engen wrote: > > > > Acked-by: Jeremy Kerr > > Tested-by: Jeremy Kerr > > Signed-off-by: Dwight Engen > > Reviewed-by: Arnd Bergmann Applied. Thanks, -Ben ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 01/31] spi: mpc512x: cleanup clock API use
On Wed, 21 Aug 2013 20:48:17 +0100 Mark Brown wrote: > On Wed, Aug 21, 2013 at 09:22:58PM +0200, Anatolij Gustschin wrote: > > > Mark, are you going to apply this patch? Or should I queue it > > in my mpc5xxx tree (I'd like to get your Acked-by then)? > > Has this series settled down? I'd been ignoring it since it was getting > so many and so frequent revisions. Patches 01 - 14 (except 09 and 11) won't change I think. I'd prefer to queue them for v3.12, so there will be no need to resubmit again and again. Other patches are not ready yet. Anatolij ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 03/31] USB: fsl-mph-dr-of: cleanup clock API use
On Tue, 6 Aug 2013 22:43:43 +0200 Gerhard Sittig wrote: > use devm_get_clk() for automatic put upon device close, check for and > propagate errors when enabling clocks, must prepare clocks before they > can get enabled, unprepare after disable > > Signed-off-by: Gerhard Sittig > --- > drivers/usb/host/fsl-mph-dr-of.c | 16 +--- > 1 file changed, 9 insertions(+), 7 deletions(-) applied, thanks! Anatolij ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote: > On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote: > > Quoting Mark Rutland (2013-08-19 02:35:43) > > > > > On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote: > > > > On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: > > > > > On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: > > > > > > > > > Also I would make this option required. Use a dummy > > > > > > > > > clock for > > > > > > > > > mux > > > > > > > > > inputs that are grounded for a specific SoC. > > > > > > > > > > > > > > > > Some clocks are not from CCM and we haven't defined in > > > > > > > > imx6q-clk.txt, > > > > > > > > so in most cases we can't provide a phandle for them, eg: > > > > > > > > spdif_ext. > > > > > > > > I think it's a bit hard to force it to be 'required'. An > > > > > > > > 'optional' > > > > > > > > looks more flexible to me and a default one is ensured > > > > > > > > even if > > > > > > > > it's > > > > > > > > missing. > > > > > > > > > > > > > > <&clks 0> is the dummy clock. This can be used for all input > > > > > > > clocks > > > > > > > not > > > > > > > defined by the SoC. > > > > > > > > > > > > Where does this assumption come from? Is it documented > > > > > > anywhere? > > > > > > > > > > This is how all i.MX clock bindings currently are. See > > > > > Documentation/devicetree/bindings/clock/imx*-clock.txt > > > > > > > > OK, thanks. > > > > > > > > I guess we need some discussion on dummy clocks vs skipped clocks. > > > > I think we want some consistency on this, don't we? > > > > > > > > If we really need a dummy clock, then we might also want a generic > > > > way to specify it. > > > > > > What do we actually mean by a "dummy clock"? We already have > > > bindings > > > for "fixed-clock" and co friends describe relatively simple > > > preconfigured clocks. > > > > Some platforms have a fake clock which defines noops callbacks and > > basically doesn't do anything. This is analogous to the dummy > > regulator > > implementation. A central one could be registered by the clock core, > > as > > is done by the regulator core. > > When you say some platforms, you presumably mean the platform code in > Linux? A dummy clock sounds like a completely Linux-specific abstraction > rather than a description of the hardware, and I don't see why we need > that in the DT: > > * If a clock is wired up and running (as presumably the dummy clock is), > then surely it's a fixed-clock (it's running, we and we have no control > over it, but we presumably know its rate) and can be described as such? > > * If no clock is wired up, then we should be able to describe that. If a > driver believes that a clock is required when it isn't (for some level > of functionality), then that driver should be fixed up to support the > clock as being optional. > > Am I missing something? I second that. Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available. Best regards, Tomasz ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v10 2/2] ASoC: fsl: Add S/PDIF machine driver
On 08/21/2013 12:54 PM, Tomasz Figa wrote: > On Wednesday 21 of August 2013 12:30:59 Stephen Warren wrote: >> On 08/20/2013 09:13 PM, Nicolin Chen wrote: >>> This patch implements a device-tree-only machine driver for Freescale >>> i.MX series Soc. It works with spdif_transmitter/spdif_receiver and >>> fsl_spdif.c drivers. >>> >>> diff --git >>> a/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt >>> b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt >>> >>> +Optional properties: >>> + >>> + - spdif-transmitter : The phandle of the spdif-transmitter codec >>> + >>> + - spdif-receiver : The phandle of the spdif-receiver codec >>> + >>> +* Note: At least one of these two properties should be set in the DT >>> binding. >> I still don't think those two properties are correct. >> >> Exactly what node will those phandles point at? > > Imagine following setup: > > > || RX | Microphone DSP | Analog mic input > | S/PDIF | << || <--- > || > | DAI | >> | Amplifier | >--- > || TX || Speakers output > > As you see in the diagram, the S/PDIF interface of the SoC can be > connected to some external devices that can perform sound processing or > simply handle the physical layer. > > I'd say that normally both RX and TX lines would be connected to a single > codec chip that has multiple blocks inside, like sound processing, > amplifier, mixer, etc., but nothing stops you from making a crazy setup, > when RX and TX lines are connected to different chips. That's much rarer with S/PDIF than I2S though right? Usually I'd expect the S/PDIF controller to simply be routed out to a jack/connector on the board, or perhaps to an internal HDMI encoder. But the point of my question was more that the binding should fully describe the type of object/node that the phandle should reference. The type of the node would then imply what kind of operations could be performed on that other node's driver by this node's driver. If the set of operations is undefined, that's bad. If the set of operations is NULL, there's probably no need to reference the node. >> There definitely should not be a DT node for any "dummy CODEC", >> irrespective of whether this binding calls the other node a "CODEC" or a >> "dummy CODEC". > > I agree. Instead if no chip connected to particular line is specified in > device tree, it's responsibility of Linux sound core to handle this > properly by adding a dummy codec or whatever. > >> If these properties are to contain phandles, it would be acceptable for >> the referenced node to be: >> >> * A node representing the physical connector/jack on the board. >> >> * A node representing some other IP block on the board, such as an HDMI >> encoder/display-controller >> >> I think those options are unlikely in general > > Why? You usually codec SoC DAIs to some external chips. It's unlikely the phandle would reference a connector/jack since we (thus far at least) haven't created DT nodes for them. If we do put jacks/connectors into DT, then referencing them is fine. In the "other IP block" case, it may be worth referencing the device, although as I mentioned above, we need to describe exactly what is expected from that other device interface-wise. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes
On 07/29/2013 04:49 AM, hongbo.zh...@freescale.com wrote:
> Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this patch adds
> the device tree nodes for them.
> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
> b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
> +** Freescale Elo3 DMA Controller
> + This is EloPlus controller with 8 channels, used in Freescale Txxx and
> Bxxx
> + series chips, such as t1040, t4240, b4860.
> +
> +Required properties:
> +
> +- compatible: must include "fsl,elo3-dma"
This should probably list all the SoC-specific compatible values too.
> +- ranges: describes the mapping between the address space of the
> + DMA channels and the address space of the DMA
> controller
Oh, so looking at the example, this is simply about being able to write
the reg value in the child nodes more easily without having to write out
the full based address of the controller in each child node.
I don't think the binding document should require this; all the binding
document should care about is that the child nodes have a valid reg
value. Whether that reg value is <0x100100 0x80> without a ranges in the
top-level DMA nor or whether that reg value is <0x0 0x80> with a ranges
value in the top-level DMA node isn't something that the binding should
specify. Either way will work equally without affecting a driver for the
DMA controller; the parsing of reg with/without a ranges property is
more of a core part of DT than anything to do with this binding.
> +- DMA channel nodes:
> +- compatible: must include "fsl,eloplus-dma-channel"
Why do the channel nodes even need a compatible value? Presumably the
driver for the top-level DMA node will scan these dma-channel nodes to
extract the information it needs and will simply assume that all these
nodes are DMA channel nodes rather than something else? I suppose this
doesn't hurt, it just seems unnecessary unless you foresee other child
nodes types existing in the future and hence a need to differentiate
different types of nodes.
> +- reg :
> +- interrupts:
s/interrupts/specifier/
> +Example:
> +dma@100300 {
> + #address-cells = <1>;
> + #size-cells = <1>;
Those weren't mentioned in the required properties list above. Perhaps
they're considered such a core part of DT functionality that it's not
necessary, yet some other binding documents do include these properties
in the list of required properties.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document
On 07/29/2013 04:49 AM, hongbo.zh...@freescale.com wrote: > From: Hongbo Zhang > > This patch updates the discription of each type of DMA controller and its > channels, it is preparation for adding another new DMA controller binding, it > also fixes some defects of indent for text alignment at the same time. > diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt > b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt > -- compatible: compatible list, contains 2 entries, first is > - "fsl,CHIP-dma", where CHIP is the processor > - (mpc8349, mpc8360, etc.) and the second is > - "fsl,elo-dma" > +- compatible: must include "fsl,elo-dma" Why remove the list of supported compatible values. Lately it seems that we're moving towards listing more/all the values rather than removing their documentation... > -- ranges : Should be defined as specified in 1) to describe the > - DMA controller channels. > +- ranges: describes the mapping between the address space of the > + DMA channels and the address space of the DMA > controller What is "the address space of the DMA controller". Perhaps this should say "the CPU-visible address space" instead? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document
On Wed, 2013-08-21 at 16:33 -0600, Stephen Warren wrote: > On 07/29/2013 04:49 AM, hongbo.zh...@freescale.com wrote: > > From: Hongbo Zhang > > > > This patch updates the discription of each type of DMA controller and its > > channels, it is preparation for adding another new DMA controller binding, > > it > > also fixes some defects of indent for text alignment at the same time. > > > diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt > > b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt > > > -- compatible: compatible list, contains 2 entries, first is > > -"fsl,CHIP-dma", where CHIP is the processor > > -(mpc8349, mpc8360, etc.) and the second is > > -"fsl,elo-dma" > > +- compatible: must include "fsl,elo-dma" > > Why remove the list of supported compatible values. Lately it seems that > we're moving towards listing more/all the values rather than removing > their documentation... Previous versions had language that required fsl,CHIP-dma for 83xx (and maybe 85xx?) but not the new chip. I asked for it to be consistent. The reason that 83xx still has fsl,CHIP-dma is not because of anything special to 83xx, but that most other chips with this device have been converted to dtsi and it's much more of a pain to specify the specific SoC in that context. The existing language does not match actual device trees when it comes to 85xx. Plus, the exact SoC name is of dubious value for integrated devices. It doesn't uniquely identify the hardware because different versions of the SoC could have different versions of the subdevice. As such, on our chips we've been moving away from including a compatible that specifies the exact SoC. If it turns out we made a mistake in naming different versions of the device, or if there are errata, the exact SoC can still be determined at runtime using SVR. > > -- ranges : Should be defined as specified in 1) to describe the > > - DMA controller channels. > > +- ranges: describes the mapping between the address space of > > the > > + DMA channels and the address space of the DMA > > controller > > What is "the address space of the DMA controller". Perhaps this should > say "the CPU-visible address space" instead? It's translating from the addresses used in the child nodes to a CCSR offset. It's really just a convenience for the readability and macro-ability of the device tree that we do this translation at all, versus having an empty ranges and using CCSR offsets in the children. It's not about translating between the DMA controller's view and the CPU's view or anything like that. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes
On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote:
> On 07/29/2013 04:49 AM, hongbo.zh...@freescale.com wrote:
> > Freescale QorIQ T4 and B4 introduce new 8-channel DMA engines, this patch
> > adds
> > the device tree nodes for them.
>
> > diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
> > b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt
>
> > +** Freescale Elo3 DMA Controller
> > + This is EloPlus controller with 8 channels, used in Freescale Txxx and
> > Bxxx
> > + series chips, such as t1040, t4240, b4860.
> > +
> > +Required properties:
> > +
> > +- compatible: must include "fsl,elo3-dma"
>
> This should probably list all the SoC-specific compatible values too.
We're not going to specify them in the device tree (see my comment on
patch 1/3), so we probably shouldn't lie about them in the binding like
eloplus currently does. :-)
> > +- ranges: describes the mapping between the address space of
> > the
> > + DMA channels and the address space of the DMA
> > controller
>
> Oh, so looking at the example, this is simply about being able to write
> the reg value in the child nodes more easily without having to write out
> the full based address of the controller in each child node.
>
> I don't think the binding document should require this;
It doesn't. It just requires that there be a mapping; it doesn't have
to be any particular mapping.
> all the binding document should care about is that the child nodes have a
> valid reg
> value. Whether that reg value is <0x100100 0x80> without a ranges in the
> top-level DMA
Without a ranges property there is no translation and the registers
would not be memory mappable. Linux may treat the absence of ranges as
an identity mapping for compatibility with some broken OF trees, but
it's not standard.
> nor or whether that reg value is <0x0 0x80> with a ranges
> value in the top-level DMA node isn't something that the binding should
> specify. Either way will work equally without affecting a driver for the
> DMA controller; the parsing of reg with/without a ranges property is
> more of a core part of DT than anything to do with this binding.
>
> > +- DMA channel nodes:
> > +- compatible: must include "fsl,eloplus-dma-channel"
>
> Why do the channel nodes even need a compatible value? Presumably the
> driver for the top-level DMA node will scan these dma-channel nodes to
> extract the information it needs and will simply assume that all these
> nodes are DMA channel nodes rather than something else? I suppose this
> doesn't hurt, it just seems unnecessary unless you foresee other child
> nodes types existing in the future and hence a need to differentiate
> different types of nodes.
Other than "this is how the existing binding works and we're not going
to break compatibility", it allows the OS more flexibility to choose
whether to bind to controllers or directly to the channels. Sometimes a
channel will be labelled with a different compatible if it has a fixed
purpose such as being connected to audio hardware (e.g. mpc8610_hpcd.dts
where some channels are "fsl,ssi-dma-channel").
The channels are mostly independent. Only an interrupt is shared on
elo, and only an IOMMU device number on eloplus/elo3 -- plus a shared
status register that doesn't have to be used and doesn't make sense to
use without a shared interrupt.
> > +- reg :
> > +- interrupts:
>
> s/interrupts/specifier/
>
> > +Example:
> > +dma@100300 {
> > + #address-cells = <1>;
> > + #size-cells = <1>;
>
> Those weren't mentioned in the required properties list above.
It's inherent in the existence of child nodes with reg (unless you rely
on the default of 2/1, which is discouraged). The binding should
mention it if it has particular requirements for the value of either
property, but I don't think we care here (much like we don't care what
sort of translation is used in ranges).
> Perhaps they're considered such a core part of DT functionality that it's not
> necessary, yet some other binding documents do include these properties
> in the list of required properties.
Some bindings even try to repeat the definition of standard properties
-- often incorrectly. We should avoid that.
-Scott
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes
On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote: > On 07/29/2013 04:49 AM, hongbo.zh...@freescale.com wrote: > > +- reg : > > +- interrupts: > > s/interrupts/specifier/ Do you mean s/interrupt mapping/interrupt specifier/? And probably s/registers mapping/register specifier/ as well. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document
On 08/21/2013 04:45 PM, Scott Wood wrote: > On Wed, 2013-08-21 at 16:33 -0600, Stephen Warren wrote: >> On 07/29/2013 04:49 AM, hongbo.zh...@freescale.com wrote: >>> From: Hongbo Zhang >>> >>> This patch updates the discription of each type of DMA controller and its >>> channels, it is preparation for adding another new DMA controller binding, >>> it >>> also fixes some defects of indent for text alignment at the same time. >> >>> diff --git a/Documentation/devicetree/bindings/powerpc/fsl/dma.txt >>> b/Documentation/devicetree/bindings/powerpc/fsl/dma.txt >> >>> -- compatible: compatible list, contains 2 entries, first is >>> -"fsl,CHIP-dma", where CHIP is the processor >>> -(mpc8349, mpc8360, etc.) and the second is >>> -"fsl,elo-dma" >>> +- compatible: must include "fsl,elo-dma" >> >> Why remove the list of supported compatible values. Lately it seems that >> we're moving towards listing more/all the values rather than removing >> their documentation... > > Previous versions had language that required fsl,CHIP-dma for 83xx (and > maybe 85xx?) but not the new chip. I asked for it to be consistent. > The reason that 83xx still has fsl,CHIP-dma is not because of anything > special to 83xx, but that most other chips with this device have been > converted to dtsi and it's much more of a pain to specify the specific > SoC in that context. The existing language does not match actual device > trees when it comes to 85xx. > > Plus, the exact SoC name is of dubious value for integrated devices. It > doesn't uniquely identify the hardware because different versions of the > SoC could have different versions of the subdevice. As such, on our > chips we've been moving away from including a compatible that specifies > the exact SoC. If it turns out we made a mistake in naming different > versions of the device, or if there are errata, the exact SoC can still > be determined at runtime using SVR. OK, if there's some alternative run-time way of enabling chip-specific quirking, it's probably fine to remove the extra compatible values. Now, that does rather assume that this DMA IP block will only ever be used within SoCs that have that SVR concept, but perhaps if that's ever not the case, we can simply go back to requiring extra compatible values in those specific cases? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes
On 08/21/2013 05:00 PM, Scott Wood wrote: > On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote: >> On 07/29/2013 04:49 AM, hongbo.zh...@freescale.com wrote: >>> +- reg : >>> +- interrupts: >> >> s/interrupts/specifier/ > > Do you mean s/interrupt mapping/interrupt specifier/? > > And probably s/registers mapping/register specifier/ as well. Yup. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes
On 08/21/2013 04:57 PM, Scott Wood wrote: > On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote: >> On 07/29/2013 04:49 AM, hongbo.zh...@freescale.com wrote: >>> +- ranges: describes the mapping between the address space of >>> the >>> + DMA channels and the address space of the DMA >>> controller >> >> Oh, so looking at the example, this is simply about being able to write >> the reg value in the child nodes more easily without having to write out >> the full based address of the controller in each child node. >> >> I don't think the binding document should require this; > > It doesn't. It just requires that there be a mapping; it doesn't have > to be any particular mapping. > >> all the binding document should care about is that the child nodes have a >> valid reg >> value. Whether that reg value is <0x100100 0x80> without a ranges in the >> top-level DMA > > Without a ranges property there is no translation and the registers > would not be memory mappable. Linux may treat the absence of ranges as > an identity mapping for compatibility with some broken OF trees, but > it's not standard. I would argue that missing ranges meaning 1:1 translation is now a standard, given that it must be true to support some DTs, it therefore can now be assumed? >> nor or whether that reg value is <0x0 0x80> with a ranges >> value in the top-level DMA node isn't something that the binding should >> specify. Either way will work equally without affecting a driver for the >> DMA controller; the parsing of reg with/without a ranges property is >> more of a core part of DT than anything to do with this binding. >> >>> +- DMA channel nodes: >>> +- compatible: must include "fsl,eloplus-dma-channel" >> >> Why do the channel nodes even need a compatible value? Presumably the >> driver for the top-level DMA node will scan these dma-channel nodes to >> extract the information it needs and will simply assume that all these >> nodes are DMA channel nodes rather than something else? I suppose this >> doesn't hurt, it just seems unnecessary unless you foresee other child >> nodes types existing in the future and hence a need to differentiate >> different types of nodes. > > Other than "this is how the existing binding works and we're not going > to break compatibility", it allows the OS more flexibility to choose > whether to bind to controllers or directly to the channels. Sometimes a > channel will be labelled with a different compatible if it has a fixed > purpose such as being connected to audio hardware (e.g. mpc8610_hpcd.dts > where some channels are "fsl,ssi-dma-channel"). That sounds terribly like encoding policy into DT rather than it being a HW description. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes
On Wed, 2013-08-21 at 17:15 -0600, Stephen Warren wrote: > On 08/21/2013 04:57 PM, Scott Wood wrote: > > On Wed, 2013-08-21 at 16:40 -0600, Stephen Warren wrote: > >> On 07/29/2013 04:49 AM, hongbo.zh...@freescale.com wrote: > > >>> +- ranges: describes the mapping between the address space of > >>> the > >>> + DMA channels and the address space of the DMA > >>> controller > >> > >> Oh, so looking at the example, this is simply about being able to write > >> the reg value in the child nodes more easily without having to write out > >> the full based address of the controller in each child node. > >> > >> I don't think the binding document should require this; > > > > It doesn't. It just requires that there be a mapping; it doesn't have > > to be any particular mapping. > > > >> all the binding document should care about is that the child nodes have a > >> valid reg > >> value. Whether that reg value is <0x100100 0x80> without a ranges in the > >> top-level DMA > > > > Without a ranges property there is no translation and the registers > > would not be memory mappable. Linux may treat the absence of ranges as > > an identity mapping for compatibility with some broken OF trees, but > > it's not standard. > > I would argue that missing ranges meaning 1:1 translation is now a > standard, given that it must be true to support some DTs, it therefore > can now be assumed? "Some broken tree does it therefore it's fine for everyone to do it" is awful. We have standards documents for a reason. Plus, not all OSes need to run on hardware with that broken firmware. For example, the Freescale Embedded Hypervisor will not accept a missing ranges in that way. U-Boot does accept it, but only because the code was copied from Linux (including the comment that says it's not supposed to work that way). If anything, we should remove the hack from U-Boot, and fix Linux to only apply it in situations where it's known to be needed. > >> Why do the channel nodes even need a compatible value? Presumably the > >> driver for the top-level DMA node will scan these dma-channel nodes to > >> extract the information it needs and will simply assume that all these > >> nodes are DMA channel nodes rather than something else? I suppose this > >> doesn't hurt, it just seems unnecessary unless you foresee other child > >> nodes types existing in the future and hence a need to differentiate > >> different types of nodes. > > > > Other than "this is how the existing binding works and we're not going > > to break compatibility", it allows the OS more flexibility to choose > > whether to bind to controllers or directly to the channels. Sometimes a > > channel will be labelled with a different compatible if it has a fixed > > purpose such as being connected to audio hardware (e.g. mpc8610_hpcd.dts > > where some channels are "fsl,ssi-dma-channel"). > > That sounds terribly like encoding policy into DT rather than it being a > HW description. It is hardware description. Those DMA channels are physically wired into the audio hardware. Other DMA channels in the same system aren't. The only thing that's even slightly policy is that it assumes you aren't going to ignore the audio device altogether, and use it as an extra generic DMA channel. I'm not sure that it's worthwhile to care in this case. If you want to be 100% policy-free then we shouldn't be specifying a lot of the addresses we currently do, since that's actually just how U-Boot configured things. Sometimes simplifying assumptions get made, when what the hardware people actually came up with is too awkward to be worth describing directly. In any case, this is not new, nor is it relevant to the hardware we're currently adding support for, and we're not going to break compatibility now. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 1/3] DMA: Freescale: revise device tree binding document
On Wed, 2013-08-21 at 17:12 -0600, Stephen Warren wrote: > OK, if there's some alternative run-time way of enabling chip-specific > quirking, it's probably fine to remove the extra compatible values. > > Now, that does rather assume that this DMA IP block will only ever be > used within SoCs that have that SVR concept, but perhaps if that's ever > not the case, we can simply go back to requiring extra compatible values > in those specific cases? The only situation I can see where SVR would be absent is if we were to integrate this device into an ARM chip, in which case I'd expect there to be some equivalent way to find the SoC identification. If the driver knows what SoC version it expects, it will know the way that that SoC advertises its version. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 2/3] DMA: Freescale: Add new 8-channel DMA engine device tree nodes
On Wed, Aug 21, 2013 at 6:31 PM, Scott Wood wrote: > >> > Other than "this is how the existing binding works and we're not going >> > to break compatibility", it allows the OS more flexibility to choose >> > whether to bind to controllers or directly to the channels. Sometimes a >> > channel will be labelled with a different compatible if it has a fixed >> > purpose such as being connected to audio hardware (e.g. mpc8610_hpcd.dts >> > where some channels are "fsl,ssi-dma-channel"). >> >> That sounds terribly like encoding policy into DT rather than it being a >> HW description. > > It is hardware description. Those DMA channels are physically wired > into the audio hardware. Other DMA channels in the same system aren't. Well, not quite. Technically the DMA channel can be dynamically assigned to the SSI, but there are limits. At the time the code was written, there was no way to reserve a DMA channel from the generic DMA driver, and I didn't want to have to depend on that driver either. Using the device tree forced a specific pair of channels to be assigned to each SSI. The audio driver has code to program the SoC to route whichever DMA channels are assigned, but it assumes that the device tree has a valid assignment. I believe the generic DMA driver can now accept DMA channel reservations, but I don't think it works both ways. That is, if the audio driver loads first, I don't think there's a clean way to tell the DMA driver which channels have already been taken by the audio driver. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH 1/1] powerpc/embedded6xx: Add support for Motorola/Emerson MVME5100.
Scott Wood wrote on 08/21/2013 09:20:03 AM:
> From: Scott Wood
> To: Stephen N Chivers
> Cc: , Chris Proctor ,
> ,
> Date: 08/21/2013 09:20 AM
> Subject: Re: [RFC PATCH 1/1] powerpc/embedded6xx: Add support for
> Motorola/Emerson MVME5100.
>
> On Tue, 2013-08-20 at 13:28 +1100, Stephen N Chivers wrote:
> > Scott Wood wrote on 08/09/2013 11:35:20 AM:
> >
> > > From: Scott Wood
> > > To: Stephen N Chivers
> > > Cc: , , Chris Proctor
> > > ,
> > > Date: 08/09/2013 11:36 AM
> > > Subject: Re: [RFC PATCH 1/1] powerpc/embedded6xx: Add support for
> > > Motorola/Emerson MVME5100.
> > >
> > > simple-bus may be applicable here (in addition to a specific
> > > compatible).
> > >
> > The HAWK ASIC is a difficult beast. I still cannot get a positive
> > identification as to what it is (Motorola/Freescale part number
> > unknown, not even the part number on the chip on the board helps).
> > The best I can come up with is that it is a tsi108 without
> > the ethenets.
> > So device_type will be tsi-bridge and compatible will be
> > tsi108-bridge.
>
> Don't use device_type. compatible should include "hawk" in the name
> (especially if you're not sure what's really in it), and/or the part
> number on the chip. If you're convinced it's fully compatible with
> tsi108-bridge you can add that as a second compatible value, though
> given the uncertainty it's probably better to just teach Linux to look
> for the new compatible.
>
> If devices on the bus can be used without any special bus setup or
> knowledge, then you can add a compatible of "simple-bus" to the end.
>
> > > Why not just look for a chrp,iic node directly?
> > >
> > I was following the model used in other places, like chrp/setup.c.
>
> Not all examples are good examples. :-)
>
> > > > + if ((np = of_find_compatible_node(NULL, "pci",
"mpc10x-pci")))
> > {
> > >
> > > Why insist on the device_type?
> > >
> > Following the model in the linkstation (kurobox) platform support.
>
> Drop the device_type check.
>
> > > > +static void
> > > > +mvme5100_restart(char *cmd)
> > > > +{
> > > > + volatile ulong i = 1000;
> > > > +
> > > > +
> > > > + local_irq_disable();
> > > > + _nmask_and_or_msr(0, MSR_IP);
> > >
> > > Does "mtmsr(mfmsr() | MSR_IP)" not work?
> > >
> > Don't know. Is from the original code by Matt Porter.
>
> It actually appears that there are no callers remaining that use the
> "and" portion of the functionality. In fact there are no callers that
> use it for anything other than setting MSR_IP. :-P
>
> > > > + out_8((u_char *) BOARD_MODRST_REG, 0x01);
> > > > +
> > > > + while (i-- > 0);
> > >
> > > Do not use a loop to implement a delay.
> > >
> > Taken from the original code. But at this point the board
> > is going to reset and reboot via firmware, as /sbin/reboot
> > or /sbin/halt has been invoked.
>
> Still, it's just a bad idea. What's wrong with udelay()?
>
> Or just use an infinite loop. How much value is there really in timing
> out here?
>
> > > > +static void __init
> > > > +mvme5100_set_bat(void)
> > > > +{
> > > > +
> > > > +
> > > > + mb();
> > > > + mtspr(SPRN_DBAT1U, 0xf0001ffe);
> > > > + mtspr(SPRN_DBAT1L, 0xf02a);
> > > > + mb();
> > > > + setbat(1, 0xfe00, 0xfe00, 0x0200,
> > PAGE_KERNEL_NCG);
> > > > +}
> > >
> > > It is no longer allowed to squat on random virtual address space
like
> > > this. If you really need a BAT you'll have to allocate the virtual
> > > address properly.
> > >
> > Yes. I found that this was an anathema when researching the port in
> > 2010 but I couldn't find any practical solution at the time.
> > The code is called early to ensure that the hawk registers are
available.
> > sysdev/cpm_common.c does the same thing.
>
> > What is the correct solution?
>
> ioremap() has special code to function early (using ioremap_bot).
>
> If you still need to use a BAT that early, reserve the space with
> asm/fixmap.h or by adding a function to the early ioremap code to just
> reserve the space. Or better, improve the ioremap code to be capable of
> creating a BAT (or equivalent) when requested.
>
It is really interesting. Given that the UART implementation on the
HAWK is such that legacy_serial will not set up an early console it
is very likely that the address translation set up by the bat is not
required.
I can probably replace the physical addresses used in:
setup_indirec_pci(hose, 0, 0xfe000cf8, 0xfe000cfc, 0);
with remapped equivalents. But, with the setbat eliminated, the
line:
pcibios_alloc_controller(dev);
silently (remember no early console, due to UART reg-shift) panics.
It is happening at the point where the newly allocated PHB
structure is being added to the "hose_list" in pci-common.c.
So, I think there is some side effect due to the call to the setbat with
the
PAGE_KERNEL_NCG parameter that I do not yet understand.
> -Scott
>
>
>
___
[PATCH v2] powerpc: purge all the prefetched instructions for the coherent icache flush
As Benjamin Herrenschmidt has indicated, we still need a dummy icbi to
purge all the prefetched instructions from the ifetch buffers for the
snooping icache. We also need a sync before the icbi to order the
actual stores to memory that might have modified instructions with
the icbi.
Signed-off-by: Kevin Hao
---
arch/powerpc/include/asm/cache.h | 14 +-
arch/powerpc/kernel/misc_32.S| 4 +++-
arch/powerpc/kernel/misc_64.S| 3 ++-
3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/include/asm/cache.h b/arch/powerpc/include/asm/cache.h
index 9e495c9..ed0afc1 100644
--- a/arch/powerpc/include/asm/cache.h
+++ b/arch/powerpc/include/asm/cache.h
@@ -41,8 +41,20 @@ struct ppc64_caches {
extern struct ppc64_caches ppc64_caches;
#endif /* __powerpc64__ && ! __ASSEMBLY__ */
-#if !defined(__ASSEMBLY__)
+#if defined(__ASSEMBLY__)
+/*
+ * For a snooping icache, we still need a dummy icbi to purge all the
+ * prefetched instructions from the ifetch buffers. We also need a sync
+ * before the icbi to order the the actual stores to memory that might
+ * have modified instructions with the icbi.
+ */
+#define PURGE_PREFETCHED_INS \
+ sync; \
+ icbi0,r3; \
+ sync; \
+ isync
+#else
#define __read_mostly __attribute__((__section__(".data..read_mostly")))
#ifdef CONFIG_6xx
diff --git a/arch/powerpc/kernel/misc_32.S b/arch/powerpc/kernel/misc_32.S
index 777d999..d1e819b 100644
--- a/arch/powerpc/kernel/misc_32.S
+++ b/arch/powerpc/kernel/misc_32.S
@@ -329,7 +329,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_UNIFIED_ID_CACHE)
*/
_KPROBE(flush_icache_range)
BEGIN_FTR_SECTION
- isync
+ PURGE_PREFETCHED_INS
blr /* for 601, do nothing */
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
li r5,L1_CACHE_BYTES-1
@@ -433,6 +433,7 @@ _GLOBAL(invalidate_dcache_range)
*/
_GLOBAL(__flush_dcache_icache)
BEGIN_FTR_SECTION
+ PURGE_PREFETCHED_INS
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
rlwinm r3,r3,0,0,31-PAGE_SHIFT /* Get page base address */
@@ -474,6 +475,7 @@ END_MMU_FTR_SECTION_IFSET(MMU_FTR_TYPE_44x)
*/
_GLOBAL(__flush_dcache_icache_phys)
BEGIN_FTR_SECTION
+ PURGE_PREFETCHED_INS
blr /* for 601, do nothing */
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
mfmsr r10
diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S
index 992a78e..f9e8770 100644
--- a/arch/powerpc/kernel/misc_64.S
+++ b/arch/powerpc/kernel/misc_64.S
@@ -69,6 +69,7 @@ PPC64_CACHES:
_KPROBE(flush_icache_range)
BEGIN_FTR_SECTION
+ PURGE_PREFETCHED_INS
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
/*
@@ -208,7 +209,7 @@ _GLOBAL(flush_inval_dcache_range)
*/
_GLOBAL(__flush_dcache_icache)
BEGIN_FTR_SECTION
- isync
+ PURGE_PREFETCHED_INS
blr
END_FTR_SECTION_IFSET(CPU_FTR_COHERENT_ICACHE)
/*
--
1.8.3.1
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC V2 PATCH 2/6] powerpc: Implement broadcast timer interrupt as an IPI message
On Wed, 2013-08-14 at 17:26 +0530, Preeti U Murthy wrote:
> -static irqreturn_t unused_action(int irq, void *data)
> +static irqreturn_t timer_action(int irq, void *data)
> {
> - /* This slot is unused and hence available for use, if needed
> */
> + timer_interrupt();
> return IRQ_HANDLED;
> }
>
That means we'll do irq_enter/irq_exit twice no ? And things like
may_hard_irq_enable() are also already done by do_IRQ so you
don't need timer_interrupt() to do it again.
We probably are better off breaking timer_interrupt in two:
void __timer_interrupt(struct pt_regs * regs)
Does the current stuff between irq_enter and irq_exit, timer_interrupt
does the remaining around it and calls __timer_interrupt.
Then from timer_action, you call __timer_interrupt()
Cheers,
Ben.
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH 1/2] powerpc/85xx: add hardware automatically enter altivec idle state
> -Original Message- > From: Wood Scott-B07421 > Sent: Tuesday, August 20, 2013 8:39 AM > To: Wang Dongsheng-B40534 > Cc: Wood Scott-B07421; Kumar Gala; linuxppc-dev@lists.ozlabs.org > Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically enter > altivec idle state > > On Sun, 2013-08-18 at 21:53 -0500, Wang Dongsheng-B40534 wrote: > > Thanks for your feedback. > > > > > -Original Message- > > > From: Wood Scott-B07421 > > > Sent: Saturday, August 17, 2013 12:51 AM > > > To: Kumar Gala > > > Cc: Wang Dongsheng-B40534; linuxppc-dev@lists.ozlabs.org > > > Subject: Re: [PATCH 1/2] powerpc/85xx: add hardware automatically > enter > > > altivec idle state > > > > > > On Fri, 2013-08-16 at 06:02 -0500, Kumar Gala wrote: > > > > On Aug 16, 2013, at 2:23 AM, Dongsheng Wang wrote: > > > > > > > > > From: Wang Dongsheng > > > > > > > > > > Each core's AltiVec unit may be placed into a power savings mode > > > > > by turning off power to the unit. Core hardware will > automatically > > > > > power down the AltiVec unit after no AltiVec instructions have > > > > > executed in N cycles. The AltiVec power-control is triggered by > > > hardware. > > > > > > > > > > Signed-off-by: Wang Dongsheng > > > > > > > > Why treat this as a idle HW governor vs just some one time setup at > > > boot of the time delay? > > > > > > It is being done as one-time setup, despite the function name. > > > > > > Maybe it should be moved into __setup/restore_cpu_e6500 (BTW, we > really > > > should refactor those to reduce duplication) with the timebase bit > > > number hardcoded rather than a time in us. > > > > > The frequency of the different platforms timebase is not the same. > > It's close enough. Remember, this number is a vague initial guess, not > something that's been carefully calibrated. Though, it would make it > harder to substitute the number with one that's been more carefully > calibrated... but wouldn't different chips possibly have different > optimal delays anyway? > > > If we use it, we need to set different timebase bit on each platform. > > That is why I did not put the code in __setup/restore_cpu_e6500, I need > > use tb_ticks_per_usec to get timebase bit. So we only need to set a > time > > for each platform, and not set different timebase bit. > > It just seems wrong to have an ad-hoc mechanism for running > core-specific code when we have cputable... If we really need this, > maybe we should add a "cpu_setup_late" function pointer. > > With your patch, when does the power management register get set when > hot plugging a cpu? > Um.. I don't deal with this situation. I will fix it. __setup/restore_cpu_e6500 looks good. But only bootcpu call __setup_cpu_e6500, not on each cpu. I think this is a bug. Fix code: diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S index 2cfed9e..2a9a718 100644 --- a/arch/powerpc/kernel/head_64.S +++ b/arch/powerpc/kernel/head_64.S @@ -603,6 +603,9 @@ __secondary_start: /* Set thread priority to MEDIUM */ HMT_MEDIUM +#ifdef CONFIG_PPC_BOOK3E + bl call_setup_cpu /* Call setup_cpu for this CPU */ +#endif /* Initialize the kernel stack */ LOAD_REG_ADDR(r3, current_set) sldir28,r24,3 /* get current_set[cpu#] */ diff --git a/arch/powerpc/kernel/misc_64.S b/arch/powerpc/kernel/misc_64.S index b863b87..7d84bf4 100644 --- a/arch/powerpc/kernel/misc_64.S +++ b/arch/powerpc/kernel/misc_64.S @@ -55,6 +55,17 @@ _GLOBAL(call_handle_irq) mtlrr0 blr +_GLOBAL(call_setup_cpu) + LOAD_REG_ADDR(r4, cur_cpu_spec) + ld r4, 0(r4) + ld r5, CPU_SPEC_SETUP(r4) + + cmpwi 0, r5, 0 + beqlr + ld r5, 0(r5) + mtctr r5 + bctr + .section".toc","aw" PPC64_CACHES: .tc ppc64_caches[TC],ppc64_caches > > > As for the PVR check, the upstream kernel doesn't need to care about > > > rev1, so knowing it's an e6500 is good enough. > > > > > But AltiVec idle & PW20 cannot work on rev1 platform. > > We didn't have to deal with it? > > Upstream does not run on rev1. > :), But already have customers in the use of rev1. Why we don't need to care about that? ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC V2 PATCH 3/6] cpuidle/ppc: Add timer offload framework to support deep idle states
On Wed, 2013-08-14 at 17:26 +0530, Preeti U Murthy wrote:
> static irqreturn_t timer_action(int irq, void *data)
> {
> - timer_interrupt();
> + decrementer_timer_interrupt();
> return IRQ_HANDLED;
> }
I don't completely understand what you are doing here, but ...
> @@ -223,7 +223,7 @@ irqreturn_t smp_ipi_demux(void)
>
> #ifdef __BIG_ENDIAN
> if (all & (1 << (24 - 8 * PPC_MSG_TIMER)))
> - timer_interrupt();
> + decrementer_timer_interrupt();
Why call this decrementer_* since it's specifically *not* the
decrementer ?
Makes more sense to be called broadcast_timer_interrupt() no ?
> if (all & (1 << (24 - 8 * PPC_MSG_RESCHEDULE)))
> scheduler_ipi();
> if (all & (1 << (24 - 8 * PPC_MSG_CALL_FUNC_SINGLE)))
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index 65ab9e9..7e858e1 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -42,6 +42,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -97,8 +98,11 @@ static struct clocksource clocksource_timebase = {
>
> static int decrementer_set_next_event(unsigned long evt,
> struct clock_event_device *dev);
> +static int broadcast_set_next_event(unsigned long evt,
> + struct clock_event_device *dev);
> static void decrementer_set_mode(enum clock_event_mode mode,
>struct clock_event_device *dev);
> +static void decrementer_timer_broadcast(const struct cpumask *mask);
>
> struct clock_event_device decrementer_clockevent = {
> .name = "decrementer",
> @@ -106,13 +110,26 @@ struct clock_event_device decrementer_clockevent = {
> .irq= 0,
> .set_next_event = decrementer_set_next_event,
> .set_mode = decrementer_set_mode,
> - .features = CLOCK_EVT_FEAT_ONESHOT,
> + .broadcast = decrementer_timer_broadcast,
> + .features = CLOCK_EVT_FEAT_C3STOP | CLOCK_EVT_FEAT_ONESHOT,
> };
> EXPORT_SYMBOL(decrementer_clockevent);
>
> +struct clock_event_device broadcast_clockevent = {
> + .name = "broadcast",
> + .rating = 200,
> + .irq= 0,
> + .set_next_event = broadcast_set_next_event,
> + .set_mode = decrementer_set_mode,
Same here, why "decrementer" ? This event device is *not* the
decrementer right ?
Also, pardon my ignorance, by why do we need a separate
clock_event_device ? Ie what does that do ? I am not familiar with the
broadcast scheme and what .broadcast do in the "decrementer" one, so
you need to provide me at least with better explanations.
> + .features = CLOCK_EVT_FEAT_ONESHOT,
> +};
> +EXPORT_SYMBOL(broadcast_clockevent);
> +
> DEFINE_PER_CPU(u64, decrementers_next_tb);
> static DEFINE_PER_CPU(struct clock_event_device, decrementers);
> +static DEFINE_PER_CPU(struct clock_event_device, bc_timer);
Do we really need an additional per CPU here ? What does the bc_timer
actually represent ? The sender (broadcaster) or receiver ? In the
latter case, why does it have to differ from the decrementer ?
> +int bc_cpu;
A global with that name ? Exported ? That's gross...
> #define XSEC_PER_SEC (1024*1024)
>
> #ifdef CONFIG_PPC64
> @@ -487,6 +504,8 @@ void timer_interrupt(struct pt_regs * regs)
> struct pt_regs *old_regs;
> u64 *next_tb = &__get_cpu_var(decrementers_next_tb);
> struct clock_event_device *evt = &__get_cpu_var(decrementers);
> + struct clock_event_device *bc_evt = &__get_cpu_var(bc_timer);
> + int cpu = smp_processor_id();
> u64 now;
>
> /* Ensure a positive value is written to the decrementer, or else
> @@ -532,6 +551,10 @@ void timer_interrupt(struct pt_regs * regs)
> *next_tb = ~(u64)0;
> if (evt->event_handler)
> evt->event_handler(evt);
> + if (cpu == bc_cpu && bc_evt->event_handler) {
> + bc_evt->event_handler(bc_evt);
> + }
> +
So here, on a CPU that happens to be "bc_cpu", we call an additional
"event_handler" on every timer interrupt ? What does that handler
actually do ? How does it relate to the "broadcast" field in the
original clock source ?
> } else {
> now = *next_tb - now;
> if (now <= DECREMENTER_MAX)
> @@ -806,6 +829,20 @@ static int decrementer_set_next_event(unsigned long evt,
> return 0;
> }
>
> +/*
> + * We cannot program the decrementer of a remote CPU. Hence CPUs going into
> + * deep idle states need to send IPIs to the broadcast CPU to program its
> + * decrementer for their next local event so as to receive a broadcast IPI
> + * for the same. In order to avoid the overhead of multiple CPUs from sending
> + * IPIs, this function is a nop. Instead the broadcast CPU will handle th
Re: [RFC V2 PATCH 4/6] cpuidle/ppc: Add longnap state to the idle states on powernv
On Wed, 2013-08-14 at 17:26 +0530, Preeti U Murthy wrote:
> This patch hooks into the existing broadcast framework along with the support
> that this patchset introduces for ppc, and the cpuidle driver backend
> for powernv(posted out by Deepthi Dharwar:https://lkml.org/lkml/2013/7/23/128)
> to add sleep state as one of the deep idle states, in which the decrementer
> is switched off.
>
> However in this patch, we only emulate sleep by going into a state which does
> a nap with the decrementer interrupts disabled, termed as longnap. This
> enables
> focus on the timer broadcast framework for ppc in this series of patches ,
> which is required as a first step to enable sleep on ppc.
This is only for debug / proof of concept right ? We should use a real
sleep here.
If we need to know whether the FW supports it (PORE etc...) we shall add
a device-tree property from the FW to indicate that fact.
Cheers,
Ben.
> Signed-off-by: Preeti U Murthy
> ---
>
> arch/powerpc/platforms/powernv/processor_idle.c | 48
> +++
> 1 file changed, 47 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/platforms/powernv/processor_idle.c
> b/arch/powerpc/platforms/powernv/processor_idle.c
> index f43ad91a..9aca502 100644
> --- a/arch/powerpc/platforms/powernv/processor_idle.c
> +++ b/arch/powerpc/platforms/powernv/processor_idle.c
> @@ -9,16 +9,18 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> +#include
>
> struct cpuidle_driver powernv_idle_driver = {
> .name = "powernv_idle",
> .owner =THIS_MODULE,
> };
>
> -#define MAX_IDLE_STATE_COUNT 2
> +#define MAX_IDLE_STATE_COUNT 3
>
> static int max_idle_state = MAX_IDLE_STATE_COUNT - 1;
> static struct cpuidle_device __percpu *powernv_cpuidle_devices;
> @@ -54,6 +56,43 @@ static int nap_loop(struct cpuidle_device *dev,
> return index;
> }
>
> +/* Emulate sleep, with long nap.
> + * During sleep, the core does not receive decrementer interrupts.
> + * Emulate sleep using long nap with decrementers interrupts disabled.
> + * This is an initial prototype to test the timer offload framework for ppc.
> + * We will eventually introduce the sleep state once the timer offload
> framework
> + * for ppc is stable.
> + */
> +static int longnap_loop(struct cpuidle_device *dev,
> + struct cpuidle_driver *drv,
> + int index)
> +{
> + int cpu = dev->cpu;
> +
> + unsigned long lpcr = mfspr(SPRN_LPCR);
> +
> + lpcr &= ~(LPCR_MER | LPCR_PECE); /* lpcr[mer] must be 0 */
> +
> + /* exit powersave upon external interrupt, but not decrementer
> + * interrupt, Emulate sleep.
> + */
> + lpcr |= LPCR_PECE0;
> +
> + if (cpu != bc_cpu) {
> + mtspr(SPRN_LPCR, lpcr);
> + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &cpu);
> + power7_nap();
> + clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &cpu);
> + } else {
> + /* Wakeup on a decrementer interrupt, Do a nap */
> + lpcr |= LPCR_PECE1;
> + mtspr(SPRN_LPCR, lpcr);
> + power7_nap();
> + }
> +
> + return index;
> +}
> +
> /*
> * States for dedicated partition case.
> */
> @@ -72,6 +111,13 @@ static struct cpuidle_state
> powernv_states[MAX_IDLE_STATE_COUNT] = {
> .exit_latency = 10,
> .target_residency = 100,
> .enter = &nap_loop },
> + { /* LongNap */
> + .name = "LongNap",
> + .desc = "LongNap",
> + .flags = CPUIDLE_FLAG_TIME_VALID,
> + .exit_latency = 10,
> + .target_residency = 100,
> + .enter = &longnap_loop },
> };
>
> static int powernv_cpuidle_add_cpu_notifier(struct notifier_block *n,
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc/p1010rdb: update phy node in dts
Update phy node according to new P1010RDB-PB board.
Signed-off-by: Shengzhou Liu
---
arch/powerpc/boot/dts/p1010rdb.dtsi |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/boot/dts/p1010rdb.dtsi
b/arch/powerpc/boot/dts/p1010rdb.dtsi
index ec7c27a..da24b2d 100644
--- a/arch/powerpc/boot/dts/p1010rdb.dtsi
+++ b/arch/powerpc/boot/dts/p1010rdb.dtsi
@@ -193,17 +193,17 @@
mdio@24000 {
phy0: ethernet-phy@0 {
- interrupts = <3 1 0 0>;
+ interrupts = <0 1>;
reg = <0x1>;
};
phy1: ethernet-phy@1 {
- interrupts = <2 1 0 0>;
+ interrupts = <2 1>;
reg = <0x0>;
};
phy2: ethernet-phy@2 {
- interrupts = <2 1 0 0>;
+ interrupts = <1 1>;
reg = <0x2>;
};
--
1.7.0.4
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V4 0/5] powerpc/cpuidle: Generic POWERPC cpuidle driver enabled for POWER and POWERNV platforms
This patch series consolidates the backend cpuidle driver for pSeries and powernv platforms with minimal code duplication. Current existing backend driver for pseries has been moved to drivers/cpuidle and has been extended to accommodate powernv idle power mgmt states. As seen in V1 of this patch series, having a separate powernv backend driver results in too much code duplication, which is less elegant and can pose maintenance problems going further. Using the cpuidle framework to exploit platform low power idle states management can take advantage of advanced heuristics, tunables and features provided by framework. The statistics and tracing infrastructure provided by the cpuidle framework also helps in enabling power management related tools and help tune the system and applications. Earlier in 3.3 kernel, pSeries idle state management was modified to exploit the cpuidle framework and the end goal of this patch is to have powernv platform also to hook its idle states into cpuidle framework with minimal code duplication between both platforms. This result is a generic powerpc backend driver currently enabled for pseries and powernv platforms and which can be extended to accommodate other powerpc archs as well in the future. This series aims to maintain compatibility and functionality to existing pseries and powernv idle cpu management code. There are no new functions or idle states added as part of this series. This can be extended by adding more states to this existing framework. With this patch series, the powernv cpuidle functionalities are on-par with pSeries idle management. V1 -> http://lkml.org/lkml/2013/7/23/143 V2 -> https://lkml.org/lkml/2013/7/30/872 V3 -> http://comments.gmane.org/gmane.linux.ports.ppc.embedded/63093 Changes in V4: = * This patch series includes generic backend driver cpuidle cleanups including, replacing the driver and device initialisation routines with cpuidle_register function. * Enable CPUIDLE framework only for POWER and POWERNV platforms. Changes in V3: = * This patch series does not include smt-snooze-delay fixes. This will be taken up later on. * Integrated POWERPC driver in drivers/cpuidle. Enabled for all of POWERPC platform. Currently has PSERIES and POWERNV support. No compile time flags in .c file. This will be one consolidated binary that does a run time detection based on platform and take decisions accordingly. * Enabled CPUIDLE framwork for all of PPC64. Changes in V2: = * Merged the backend driver posted out for powernv in V1 with pSeries to create a single powerpc driver but this had compile time flags. Deepthi Dharwar (5): pseries/cpuidle: Remove dependency of pseries.h file pseries: Move plpar_wrapper.h to powerpc common include/asm location. powerpc/cpuidle: Generic powerpc backend cpuidle driver. powerpc/cpuidle: Enable powernv cpuidle support. powernv/cpuidle: Enable idle powernv cpu to call into the cpuidle framework. arch/powerpc/include/asm/paca.h | 23 + arch/powerpc/include/asm/plpar_wrappers.h | 325 + arch/powerpc/include/asm/processor.h|2 arch/powerpc/platforms/powernv/setup.c | 14 + arch/powerpc/platforms/pseries/Kconfig |9 - arch/powerpc/platforms/pseries/Makefile |1 arch/powerpc/platforms/pseries/cmm.c|3 arch/powerpc/platforms/pseries/dtl.c|3 arch/powerpc/platforms/pseries/hotplug-cpu.c|3 arch/powerpc/platforms/pseries/hvconsole.c |2 arch/powerpc/platforms/pseries/iommu.c |3 arch/powerpc/platforms/pseries/kexec.c |2 arch/powerpc/platforms/pseries/lpar.c |2 arch/powerpc/platforms/pseries/plpar_wrappers.h | 324 - arch/powerpc/platforms/pseries/processor_idle.c | 362 --- arch/powerpc/platforms/pseries/pseries.h|3 arch/powerpc/platforms/pseries/setup.c |2 arch/powerpc/platforms/pseries/smp.c|2 drivers/cpuidle/Kconfig |7 drivers/cpuidle/Makefile|2 drivers/cpuidle/cpuidle-powerpc.c | 335 + 21 files changed, 716 insertions(+), 713 deletions(-) create mode 100644 arch/powerpc/include/asm/plpar_wrappers.h delete mode 100644 arch/powerpc/platforms/pseries/plpar_wrappers.h delete mode 100644 arch/powerpc/platforms/pseries/processor_idle.c create mode 100644 drivers/cpuidle/cpuidle-powerpc.c -- Deepthi ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V4 1/5] pseries/cpuidle: Remove dependency of pseries.h file
As a part of pseries_idle cleanup to make the backend driver
code common to both pseries and powernv.
Remove non-essential smt_snooze_delay declaration in pseries.h
header file and pseries.h file inclusion in
pseries/processor_idle.c
Signed-off-by: Deepthi Dharwar
---
arch/powerpc/platforms/pseries/processor_idle.c |1 -
arch/powerpc/platforms/pseries/pseries.h|3 ---
2 files changed, 4 deletions(-)
diff --git a/arch/powerpc/platforms/pseries/processor_idle.c
b/arch/powerpc/platforms/pseries/processor_idle.c
index 4644efa0..ca70279 100644
--- a/arch/powerpc/platforms/pseries/processor_idle.c
+++ b/arch/powerpc/platforms/pseries/processor_idle.c
@@ -20,7 +20,6 @@
#include
#include "plpar_wrappers.h"
-#include "pseries.h"
struct cpuidle_driver pseries_idle_driver = {
.name = "pseries_idle",
diff --git a/arch/powerpc/platforms/pseries/pseries.h
b/arch/powerpc/platforms/pseries/pseries.h
index c2a3a25..d1b07e6 100644
--- a/arch/powerpc/platforms/pseries/pseries.h
+++ b/arch/powerpc/platforms/pseries/pseries.h
@@ -60,9 +60,6 @@ extern struct device_node *dlpar_configure_connector(u32);
extern int dlpar_attach_node(struct device_node *);
extern int dlpar_detach_node(struct device_node *);
-/* Snooze Delay, pseries_idle */
-DECLARE_PER_CPU(long, smt_snooze_delay);
-
/* PCI root bridge prepare function override for pseries */
struct pci_host_bridge;
int pseries_root_bridge_prepare(struct pci_host_bridge *bridge);
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V4 2/5] pseries: Move plpar_wrapper.h to powerpc common include/asm location.
As a part of pseries_idle backend driver cleanup to make
the code common to both pseries and powernv platforms, it
is necessary to move the backend-driver code to drivers/cpuidle.
As a pre-requisite for that, it is essential to move plpar_wrapper.h
to include/asm.
Signed-off-by: Deepthi Dharwar
---
arch/powerpc/include/asm/plpar_wrappers.h | 325 +++
arch/powerpc/platforms/pseries/cmm.c|3
arch/powerpc/platforms/pseries/dtl.c|3
arch/powerpc/platforms/pseries/hotplug-cpu.c|3
arch/powerpc/platforms/pseries/hvconsole.c |2
arch/powerpc/platforms/pseries/iommu.c |3
arch/powerpc/platforms/pseries/kexec.c |2
arch/powerpc/platforms/pseries/lpar.c |2
arch/powerpc/platforms/pseries/plpar_wrappers.h | 324 ---
arch/powerpc/platforms/pseries/processor_idle.c |3
arch/powerpc/platforms/pseries/setup.c |2
arch/powerpc/platforms/pseries/smp.c|2
12 files changed, 336 insertions(+), 338 deletions(-)
create mode 100644 arch/powerpc/include/asm/plpar_wrappers.h
delete mode 100644 arch/powerpc/platforms/pseries/plpar_wrappers.h
diff --git a/arch/powerpc/include/asm/plpar_wrappers.h
b/arch/powerpc/include/asm/plpar_wrappers.h
new file mode 100644
index 000..e2f84d6
--- /dev/null
+++ b/arch/powerpc/include/asm/plpar_wrappers.h
@@ -0,0 +1,325 @@
+#ifndef _PSERIES_PLPAR_WRAPPERS_H
+#define _PSERIES_PLPAR_WRAPPERS_H
+
+#include
+#include
+
+#include
+#include
+#include
+
+/* Get state of physical CPU from query_cpu_stopped */
+int smp_query_cpu_stopped(unsigned int pcpu);
+#define QCSS_STOPPED 0
+#define QCSS_STOPPING 1
+#define QCSS_NOT_STOPPED 2
+#define QCSS_HARDWARE_ERROR -1
+#define QCSS_HARDWARE_BUSY -2
+
+static inline long poll_pending(void)
+{
+ return plpar_hcall_norets(H_POLL_PENDING);
+}
+
+static inline u8 get_cede_latency_hint(void)
+{
+ return get_lppaca()->cede_latency_hint;
+}
+
+static inline void set_cede_latency_hint(u8 latency_hint)
+{
+ get_lppaca()->cede_latency_hint = latency_hint;
+}
+
+static inline long cede_processor(void)
+{
+ return plpar_hcall_norets(H_CEDE);
+}
+
+static inline long extended_cede_processor(unsigned long latency_hint)
+{
+ long rc;
+ u8 old_latency_hint = get_cede_latency_hint();
+
+ set_cede_latency_hint(latency_hint);
+
+ rc = cede_processor();
+#ifdef CONFIG_TRACE_IRQFLAGS
+ /* Ensure that H_CEDE returns with IRQs on */
+ if (WARN_ON(!(mfmsr() & MSR_EE)))
+ __hard_irq_enable();
+#endif
+
+ set_cede_latency_hint(old_latency_hint);
+
+ return rc;
+}
+
+static inline long vpa_call(unsigned long flags, unsigned long cpu,
+ unsigned long vpa)
+{
+ flags = flags << H_VPA_FUNC_SHIFT;
+
+ return plpar_hcall_norets(H_REGISTER_VPA, flags, cpu, vpa);
+}
+
+static inline long unregister_vpa(unsigned long cpu)
+{
+ return vpa_call(H_VPA_DEREG_VPA, cpu, 0);
+}
+
+static inline long register_vpa(unsigned long cpu, unsigned long vpa)
+{
+ return vpa_call(H_VPA_REG_VPA, cpu, vpa);
+}
+
+static inline long unregister_slb_shadow(unsigned long cpu)
+{
+ return vpa_call(H_VPA_DEREG_SLB, cpu, 0);
+}
+
+static inline long register_slb_shadow(unsigned long cpu, unsigned long vpa)
+{
+ return vpa_call(H_VPA_REG_SLB, cpu, vpa);
+}
+
+static inline long unregister_dtl(unsigned long cpu)
+{
+ return vpa_call(H_VPA_DEREG_DTL, cpu, 0);
+}
+
+static inline long register_dtl(unsigned long cpu, unsigned long vpa)
+{
+ return vpa_call(H_VPA_REG_DTL, cpu, vpa);
+}
+
+static inline long plpar_page_set_loaned(unsigned long vpa)
+{
+ unsigned long cmo_page_sz = cmo_get_page_size();
+ long rc = 0;
+ int i;
+
+ for (i = 0; !rc && i < PAGE_SIZE; i += cmo_page_sz)
+ rc = plpar_hcall_norets(H_PAGE_INIT, H_PAGE_SET_LOANED, vpa +
i, 0);
+
+ for (i -= cmo_page_sz; rc && i != 0; i -= cmo_page_sz)
+ plpar_hcall_norets(H_PAGE_INIT, H_PAGE_SET_ACTIVE,
+ vpa + i - cmo_page_sz, 0);
+
+ return rc;
+}
+
+static inline long plpar_page_set_active(unsigned long vpa)
+{
+ unsigned long cmo_page_sz = cmo_get_page_size();
+ long rc = 0;
+ int i;
+
+ for (i = 0; !rc && i < PAGE_SIZE; i += cmo_page_sz)
+ rc = plpar_hcall_norets(H_PAGE_INIT, H_PAGE_SET_ACTIVE, vpa +
i, 0);
+
+ for (i -= cmo_page_sz; rc && i != 0; i -= cmo_page_sz)
+ plpar_hcall_norets(H_PAGE_INIT, H_PAGE_SET_LOANED,
+ vpa + i - cmo_page_sz, 0);
+
+ return rc;
+}
+
+extern void vpa_init(int cpu);
+
+static inline long plpar_pte_enter(unsigned long flags,
+ unsigned long hpte_group, unsigned long hpte_v,
+ unsigned long hpte_r, unsigned long *slot)
+{
+ long rc;
+ unsigned lon
[PATCH V4 4/5] powerpc/cpuidle: Enable powernv cpuidle support.
The following patch extends the current powerpc backend
idle driver to the powernv platform.
Signed-off-by: Deepthi Dharwar
---
drivers/cpuidle/cpuidle-powerpc.c | 35 +--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/drivers/cpuidle/cpuidle-powerpc.c
b/drivers/cpuidle/cpuidle-powerpc.c
index 3662aba..973de6a 100644
--- a/drivers/cpuidle/cpuidle-powerpc.c
+++ b/drivers/cpuidle/cpuidle-powerpc.c
@@ -52,7 +52,9 @@ static int snooze_loop(struct cpuidle_device *dev,
{
unsigned long in_purr;
- idle_loop_prolog(&in_purr);
+ if (firmware_has_feature(FW_FEATURE_SPLPAR))
+ idle_loop_prolog(&in_purr);
+
local_irq_enable();
set_thread_flag(TIF_POLLING_NRFLAG);
@@ -66,7 +68,8 @@ static int snooze_loop(struct cpuidle_device *dev,
clear_thread_flag(TIF_POLLING_NRFLAG);
smp_mb();
- idle_loop_epilog(in_purr);
+ if (firmware_has_feature(FW_FEATURE_SPLPAR))
+ idle_loop_epilog(in_purr);
return index;
}
@@ -129,6 +132,15 @@ static int shared_cede_loop(struct cpuidle_device *dev,
return index;
}
+static int nap_loop(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ ppc64_runlatch_off();
+ power7_idle();
+ return index;
+}
+
/*
* States for dedicated partition case.
*/
@@ -162,6 +174,23 @@ static struct cpuidle_state
shared_states[MAX_IDLE_STATE_COUNT] = {
.enter = &shared_cede_loop },
};
+static struct cpuidle_state powernv_states[MAX_IDLE_STATE_COUNT] = {
+ { /* Snooze */
+ .name = "snooze",
+ .desc = "snooze",
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .exit_latency = 0,
+ .target_residency = 0,
+ .enter = &snooze_loop },
+ { /* NAP */
+ .name = "NAP",
+ .desc = "NAP",
+ .flags = CPUIDLE_FLAG_TIME_VALID,
+ .exit_latency = 10,
+ .target_residency = 100,
+ .enter = &nap_loop },
+};
+
void update_smt_snooze_delay(int cpu, int residency)
{
struct cpuidle_driver *drv = cpuidle_get_driver();
@@ -261,6 +290,8 @@ static int powerpc_idle_probe(void)
cpuidle_state_table = shared_states;
else if (get_lppaca_is_shared_proc() == 0)
cpuidle_state_table = dedicated_states;
+ } else if (firmware_has_feature(FW_FEATURE_OPALv3)) {
+ cpuidle_state_table = powernv_states;
} else
return -ENODEV;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH V4 3/5] powerpc/cpuidle: Generic powerpc backend cpuidle driver.
This patch involves moving the current pseries_idle backend driver code
from pseries/processor_idle.c to drivers/cpuidle/cpuidle-powerpc.c,
and making the backend code generic enough to be able to extend this
driver code for both powernv and pseries.
It enables the support for pseries platform, such that going forward the same
code
with minimal efforts can be re-used for a common driver on powernv
and can be further extended to support cpuidle idle state mgmt for the rest
of the powerpc platforms in the future. This removes a lot of code duplicacy,
making the code elegant.
Signed-off-by: Deepthi Dharwar
---
arch/powerpc/include/asm/paca.h | 23 +
arch/powerpc/include/asm/processor.h|2
arch/powerpc/platforms/pseries/Kconfig |9 -
arch/powerpc/platforms/pseries/Makefile |1
arch/powerpc/platforms/pseries/processor_idle.c | 360 ---
drivers/cpuidle/Kconfig |7
drivers/cpuidle/Makefile|2
drivers/cpuidle/cpuidle-powerpc.c | 304 +++
8 files changed, 337 insertions(+), 371 deletions(-)
delete mode 100644 arch/powerpc/platforms/pseries/processor_idle.c
create mode 100644 drivers/cpuidle/cpuidle-powerpc.c
diff --git a/arch/powerpc/include/asm/paca.h b/arch/powerpc/include/asm/paca.h
index 77c91e7..7bd83ff 100644
--- a/arch/powerpc/include/asm/paca.h
+++ b/arch/powerpc/include/asm/paca.h
@@ -175,6 +175,29 @@ extern void setup_paca(struct paca_struct *new_paca);
extern void allocate_pacas(void);
extern void free_unused_pacas(void);
+#ifdef CONFIG_PPC_BOOK3S
+#define get_lppaca_is_shared_proc() get_paca()->lppaca_ptr->shared_proc
+static inline void set_lppaca_idle(u8 idle)
+{
+ get_paca()->lppaca_ptr->idle = idle;
+}
+
+static inline void add_lppaca_wait_state(u64 cycles)
+{
+ get_paca()->lppaca_ptr->wait_state_cycles += cycles;
+}
+
+static inline void set_lppaca_donate_dedicated_cpu(u8 value)
+{
+ get_paca()->lppaca_ptr->donate_dedicated_cpu = value;
+}
+#else
+#define get_lppaca_is_shared_proc()-1
+static inline void set_lppaca_idle(u8 idle) { }
+static inline void add_lppaca_wait_state(u64 cycles) { }
+static inline void set_lppaca_donate_dedicated_cpu(u8 value) { }
+#endif
+
#else /* CONFIG_PPC64 */
static inline void allocate_pacas(void) { };
diff --git a/arch/powerpc/include/asm/processor.h
b/arch/powerpc/include/asm/processor.h
index e378ccc..5f57c56 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -430,7 +430,7 @@ enum idle_boot_override {IDLE_NO_OVERRIDE = 0,
IDLE_POWERSAVE_OFF};
extern int powersave_nap; /* set if nap mode can be used in idle loop */
extern void power7_nap(void);
-#ifdef CONFIG_PSERIES_IDLE
+#ifdef CONFIG_CPU_IDLE_POWERPC
extern void update_smt_snooze_delay(int cpu, int residency);
#else
static inline void update_smt_snooze_delay(int cpu, int residency) {}
diff --git a/arch/powerpc/platforms/pseries/Kconfig
b/arch/powerpc/platforms/pseries/Kconfig
index 62b4f80..bb59bb0 100644
--- a/arch/powerpc/platforms/pseries/Kconfig
+++ b/arch/powerpc/platforms/pseries/Kconfig
@@ -119,12 +119,3 @@ config DTL
which are accessible through a debugfs file.
Say N if you are unsure.
-
-config PSERIES_IDLE
- bool "Cpuidle driver for pSeries platforms"
- depends on CPU_IDLE
- depends on PPC_PSERIES
- default y
- help
- Select this option to enable processor idle state management
- through cpuidle subsystem.
diff --git a/arch/powerpc/platforms/pseries/Makefile
b/arch/powerpc/platforms/pseries/Makefile
index 8ae0103..4b22379 100644
--- a/arch/powerpc/platforms/pseries/Makefile
+++ b/arch/powerpc/platforms/pseries/Makefile
@@ -21,7 +21,6 @@ obj-$(CONFIG_HCALL_STATS) += hvCall_inst.o
obj-$(CONFIG_CMM) += cmm.o
obj-$(CONFIG_DTL) += dtl.o
obj-$(CONFIG_IO_EVENT_IRQ) += io_event_irq.o
-obj-$(CONFIG_PSERIES_IDLE) += processor_idle.o
ifeq ($(CONFIG_PPC_PSERIES),y)
obj-$(CONFIG_SUSPEND) += suspend.o
diff --git a/arch/powerpc/platforms/pseries/processor_idle.c
b/arch/powerpc/platforms/pseries/processor_idle.c
deleted file mode 100644
index c905b99..000
--- a/arch/powerpc/platforms/pseries/processor_idle.c
+++ /dev/null
@@ -1,360 +0,0 @@
-/*
- * processor_idle - idle state cpuidle driver.
- * Adapted from drivers/idle/intel_idle.c and
- * drivers/acpi/processor_idle.c
- *
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include
-#include
-#include
-#include
-#include
-#include
-
-struct cpuidle_driver pseries_idle_driver = {
- .name = "pseries_idle",
- .owner= THIS_MODULE,
-};
-
-#define MAX_IDLE_STATE_COUNT 2
-
-static int max_idle_state = MAX_IDLE_STATE_COUNT - 1;
-static struct cpuidle_device __percpu *pseries_cpuidle_devices;
-static s
[PATCH V4 5/5] powernv/cpuidle: Enable idle powernv cpu to call into the cpuidle framework.
This patch enables idle cpu on the powernv platform to hook on to the cpuidle
framework, if available, else call on to default idle platform
code.
Signed-off-by: Deepthi Dharwar
---
arch/powerpc/platforms/powernv/setup.c | 14 +-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powernv/setup.c
b/arch/powerpc/platforms/powernv/setup.c
index 84438af..fc62f21 100644
--- a/arch/powerpc/platforms/powernv/setup.c
+++ b/arch/powerpc/platforms/powernv/setup.c
@@ -25,6 +25,7 @@
#include
#include
#include
+#include
#include
#include
@@ -175,6 +176,17 @@ static void __init pnv_setup_machdep_rtas(void)
}
#endif /* CONFIG_PPC_POWERNV_RTAS */
+void powernv_idle(void)
+{
+ /* Hook to cpuidle framework if available, else
+* call on default platform idle code
+*/
+ if (cpuidle_idle_call()) {
+ HMT_low();
+ HMT_very_low();
+ }
+}
+
static int __init pnv_probe(void)
{
unsigned long root = of_get_flat_dt_root();
@@ -205,7 +217,7 @@ define_machine(powernv) {
.show_cpuinfo = pnv_show_cpuinfo,
.progress = pnv_progress,
.machine_shutdown = pnv_shutdown,
- .power_save = power7_idle,
+ .power_save = powernv_idle,
.calibrate_decr = generic_calibrate_decr,
#ifdef CONFIG_KEXEC
.kexec_cpu_down = pnv_kexec_cpu_down,
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH V3 3/5] powerpc/cpuidle: Generic powerpc backend cpuidle driver.
On 08/22/2013 01:38 AM, Scott Wood wrote: > On Wed, 2013-08-21 at 10:23 +0530, Deepthi Dharwar wrote: >> On 08/19/2013 11:47 PM, Scott Wood wrote: >>> On Mon, 2013-08-19 at 15:48 +0530, Deepthi Dharwar wrote: Hi Dongsheng, On 08/19/2013 11:22 AM, Wang Dongsheng-B40534 wrote: > I think we should move the states and handle function to > arch/power/platform* > The states and handle function is belong to backend driver, not for this, > different platform have different state. > Different platforms to make their own deal with these states. > > I think we cannot put all the status of different platforms and handler > in this driver. The idea here is a single powerpc back-end driver, which does a runtime detection of the platform it is running and choose the right idle states table. This was one of outcome of V2 discussion. >>> >>> I see a lot more in there than just detecting a platform and choosing a >>> driver. >>> I feel there is no harm in keeping the state information in the same file. We do have x86, which has all its variants information in one file. One place will have all the idle consolidated information of all the platform variants. If community does feel, we need to have just the states information in arch specific file, we can do so. >>> >>> What actual functionality is common to all powerpc but not common to >>> other arches? > The functionality here is idle states on powerpc like the snooze loop that is common. Also, the basic registration of the driver, hotplug notifier etc for powerpc. > >> +config CPU_IDLE_POWERPC >> +bool "CPU Idle driver for POWERPC platforms" >> +depends on PPC64 > > Why not PPC? PPC64 seems to a good place to began the consolidation work. This patch-set has not been tested for PPC32 currently. >>> >>> PPC64 is a bad place to start if you want it to be generic, because it >>> means you'll end up growing dependencies on other things that are PPC64 >>> only. There are too many arbitrary 32/64 differences as is. >> >> Hi Scott, >> >> From my understanding, PPC64 includes BOOK3E and BOOK3S archs. >> PPC includes PPC32 and PPC64. >> >> It seemed logical to start consolidating at PPC64 as >> one does not want to get into 32/64 bit differences. > > I don't want to "get into" a file that claims to be generic PPC but is > loaded with 64-bit dependencies. > >> From your comments above, I just wanted to clarify if PPC or PPC64 is >> bad place to start. If PPC64 is bad place to start, then whats the way >> forward ? Can you please throw some more light on it. > > The way forward is to give this file a more appropriate name based on > the hardware that it actually targets -- and to refactor it so that the > answer to that question is not complicated. Sure, thanks. Our idea was to have POWER archs idle states merged at the first go. Only that is what is enabled in the current version (V4 posted out) ( Code is enabled for PSERIES and POWERNV only) If needed, other POWERPC archs might benefit by extending the same driver, that is why it is named cpuidle-powerpc.c But if having cpuidle backend-driver separately for other powerpc arcs makes sense such that each one have their own state information etc then it makes sense to name the files as cpuidle-power.c, cpuilde-ppc32.c and so on. Regards, Deepthi > > -Scott > > > > ___ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev > > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC V2 PATCH 2/6] powerpc: Implement broadcast timer interrupt as an IPI message
Hi Ben
On 08/22/2013 08:40 AM, Benjamin Herrenschmidt wrote:
> On Wed, 2013-08-14 at 17:26 +0530, Preeti U Murthy wrote:
>> -static irqreturn_t unused_action(int irq, void *data)
>> +static irqreturn_t timer_action(int irq, void *data)
>> {
>> - /* This slot is unused and hence available for use, if needed
>> */
>> + timer_interrupt();
>> return IRQ_HANDLED;
>> }
>>
>
> That means we'll do irq_enter/irq_exit twice no ? And things like
> may_hard_irq_enable() are also already done by do_IRQ so you
> don't need timer_interrupt() to do it again.
>
> We probably are better off breaking timer_interrupt in two:
>
> void __timer_interrupt(struct pt_regs * regs)
>
> Does the current stuff between irq_enter and irq_exit, timer_interrupt
> does the remaining around it and calls __timer_interrupt.
>
> Then from timer_action, you call __timer_interrupt()
We actually tried out this approach. The implementation was have a
set_dec(0) in the timer_action(). This would ensure that we actually do
get a timer interrupt.
But the problem with either this approach or the one that you
suggest,i.e. calling __timer_interrupt is in the following flow.
do_IRQ() -> irq_exit() -> tick_irq_exit() -> tick_nohz_irq_exit()
-> tick_nohz_stop_sched_tick()
The problem lies in the function tick_nohz_stop_sched_tick(). This
function checks for the next timer interrupt pending on this cpu, and
programs the decrementer_next_event to the time of the next event, which
is of course > now.
As a result when in the timer_action() above, we do call
__timer_interrupt() or try to trigger a timer interrupt through
set_dec(0), the condition if(now >= *next_tb) in timer_interrupt() or
__timer_interrupt() will fail, and will not call the timer interrupt
event handler.
---> if (now >= *next_tb) {
*next_tb = ~(u64)0;
if (evt->event_handler)
evt->event_handler(evt);
} else {
The broadcast IPI , is meant to make the target CPU of this IPI believe
that it woke up from a timer interrupt, and not from an IPI. (The reason
for this I will explain in the reply to the next patch). The target CPU
should then ideally do what it would have done had it received a real
timer interrupt, call the timer interrupt event handler.
But due to the above code flow this does not happen.
Hence as the next patch PATCH 3/6 shows, we simply call the event
handler of a timer interrupt without this explicit now >= *next_tb check.
This problem arises only in the implementation of this patchset, because
a timer interrupt is pseudo triggered from an IPI. So the effects of the
IPI handler will be felt on the timer interrupt handler triggered from
this IPI, like above.
Regards
Preeti U Murthy
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH V3 3/5] powerpc/cpuidle: Generic powerpc backend cpuidle driver.
On Thu, 2013-08-22 at 11:20 +0530, Deepthi Dharwar wrote: > But if having cpuidle backend-driver separately for other powerpc arcs > makes sense such that each one have their own state information etc > then it makes sense to name the files as cpuidle-power.c, > cpuilde-ppc32.c and so on. If by "power" you mean IBM POWER machines/CPUs, then make it cpuidle-ibm-power or cpuidle-book3s64 maybe to clarify what families it affects. Cheers Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 03/19] powerpc: refactor of_get_cpu_node to support other architectures
On Tue, 2013-08-20 at 10:30 +0100, Sudeep KarkadaNagesha wrote:
> From: Sudeep KarkadaNagesha
>
> Currently different drivers requiring to access cpu device node are
> parsing the device tree themselves. Since the ordering in the DT need
> not match the logical cpu ordering, the parsing logic needs to consider
> that. However, this has resulted in lots of code duplication and in some
> cases even incorrect logic.
>
> It's better to consolidate them by adding support for getting cpu
> device node for a given logical cpu index in DT core library. However
> logical to physical index mapping can be architecture specific.
>
> PowerPC has it's own implementation to get the cpu node for a given
> logical index.
>
> This patch refactors the current implementation of of_get_cpu_node.
> This in preparation to move the implementation to DT core library.
> It separates out the logical to physical mapping so that a default
> matching of the physical id to the logical cpu index can be added
> when moved to common code. Architecture specific code can override it.
So the patch unfortunately collides with other changes in powerpc -next,
though it's not a huge deal and not hard to fixup, but expect Linus
to tick unless we sort it out some other way.
Appart from that, it's fine, builds on all my test configs and doesn't
seem to negatively impact things as far as I can tell so far...
Acked-by: Benjamin Herrenschmidt
> Cc: Rob Herring
> Cc: Grant Likely
> Cc: Benjamin Herrenschmidt
> Signed-off-by: Sudeep KarkadaNagesha
> ---
> arch/powerpc/kernel/prom.c | 76
> --
> 1 file changed, 47 insertions(+), 29 deletions(-)
>
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index eb23ac9..f7b8c0b 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -865,45 +865,63 @@ static int __init prom_reconfig_setup(void)
> __initcall(prom_reconfig_setup);
> #endif
>
> +bool arch_match_cpu_phys_id(int cpu, u64 phys_id)
> +{
> + return (int)phys_id == get_hard_smp_processor_id(cpu);
> +}
> +
> +static bool __of_find_n_match_cpu_property(struct device_node *cpun,
> + const char *prop_name, int cpu, unsigned int *thread)
> +{
> + const __be32 *cell;
> + int ac, prop_len, tid;
> + u64 hwid;
> +
> + ac = of_n_addr_cells(cpun);
> + cell = of_get_property(cpun, prop_name, &prop_len);
> + if (!cell)
> + return false;
> + prop_len /= sizeof(*cell);
> + for (tid = 0; tid < prop_len; tid++) {
> + hwid = of_read_number(cell, ac);
> + if (arch_match_cpu_phys_id(cpu, hwid)) {
> + if (thread)
> + *thread = tid;
> + return true;
> + }
> + cell += ac;
> + }
> + return false;
> +}
> +
> /* Find the device node for a given logical cpu number, also returns the cpu
> * local thread number (index in ibm,interrupt-server#s) if relevant and
> * asked for (non NULL)
> */
> struct device_node *of_get_cpu_node(int cpu, unsigned int *thread)
> {
> - int hardid;
> - struct device_node *np;
> + struct device_node *cpun, *cpus;
>
> - hardid = get_hard_smp_processor_id(cpu);
> + cpus = of_find_node_by_path("/cpus");
> + if (!cpus) {
> + pr_warn("Missing cpus node, bailing out\n");
> + return NULL;
> + }
>
> - for_each_node_by_type(np, "cpu") {
> - const u32 *intserv;
> - unsigned int plen, t;
> + for_each_child_of_node(cpus, cpun) {
> + if (of_node_cmp(cpun->type, "cpu"))
> + continue;
>
> - /* Check for ibm,ppc-interrupt-server#s. If it doesn't exist
> - * fallback to "reg" property and assume no threads
> + /* Check for non-standard "ibm,ppc-interrupt-server#s" property
> + * for thread ids on PowerPC. If it doesn't exist fallback to
> + * standard "reg" property.
>*/
> - intserv = of_get_property(np, "ibm,ppc-interrupt-server#s",
> - &plen);
> - if (intserv == NULL) {
> - const u32 *reg = of_get_property(np, "reg", NULL);
> - if (reg == NULL)
> - continue;
> - if (*reg == hardid) {
> - if (thread)
> - *thread = 0;
> - return np;
> - }
> - } else {
> - plen /= sizeof(u32);
> - for (t = 0; t < plen; t++) {
> - if (hardid == intserv[t]) {
> - if (thread)
> - *thread = t;
> - return np;
> - }
> -
Re: [RFC PATCH V3 3/5] powerpc/cpuidle: Generic powerpc backend cpuidle driver.
On 08/22/2013 11:28 AM, Benjamin Herrenschmidt wrote: > On Thu, 2013-08-22 at 11:20 +0530, Deepthi Dharwar wrote: >> But if having cpuidle backend-driver separately for other powerpc arcs >> makes sense such that each one have their own state information etc >> then it makes sense to name the files as cpuidle-power.c, >> cpuilde-ppc32.c and so on. > > If by "power" you mean IBM POWER machines/CPUs, then make it > cpuidle-ibm-power or cpuidle-book3s64 maybe to clarify what families it > affects. Sure. Thanks :) Regards, Deepthi > Cheers > Ben. > > > > ___ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev > > ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
