Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ARM: vfp: fix save and restore when running on pre-VFPv3 and CONFIG_VFPv3 set
From: Paul Walmsley @ 2012-10-13  4:46 UTC (permalink / raw)
  To: linux-arm-kernel


After commit 846a136881b8f73c1f74250bf6acfaa309cab1f2 ("ARM: vfp: fix
saving d16-d31 vfp registers on v6+ kernels"), the OMAP 2430SDP board
started crashing during boot with omap2plus_defconfig:

[    3.875122] mmcblk0: mmc0:e624 SD04G 3.69 GiB
[    3.915954]  mmcblk0: p1
[    4.086639] Internal error: Oops - undefined instruction: 0 [#1] SMP ARM
[    4.093719] Modules linked in:
[    4.096954] CPU: 0    Not tainted  (3.6.0-02232-g759e00b #570)
[    4.103149] PC is at vfp_reload_hw+0x1c/0x44
[    4.107666] LR is at __und_usr_fault_32+0x0/0x8

It turns out that the context save/restore fix unmasked a latent bug in 
commit 5aaf254409f8d58229107b59507a8235b715a960 ("ARM: 6203/1: Make VFPv3 
usable on ARMv6").  When CONFIG_VFPv3 is set, but the kernel is booted on 
a pre-VFPv3 core, the code attempts to save and restore the d16-d31 VFP 
registers.  These are only present on non-D16 VFPv3+, so the kernel dies 
with an undefined instruction exception.  The kernel didn't crash before 
commit 846a136 because the save and restore code only touched d0-d15, 
present on all VFP.

Fix to save and restore the d16-d31 registers only if they are
present.

Signed-off-by: Paul Walmsley <paul@pwsan.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Dave Martin <dave.martin@linaro.org>
---
 arch/arm/include/asm/vfp.h       |    6 ++++++
 arch/arm/include/asm/vfpmacros.h |    8 ++++----
 arch/arm/vfp/vfpmodule.c         |   10 ++++++++++
 3 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/arch/arm/include/asm/vfp.h b/arch/arm/include/asm/vfp.h
index f4ab34f..5689798 100644
--- a/arch/arm/include/asm/vfp.h
+++ b/arch/arm/include/asm/vfp.h
@@ -82,3 +82,9 @@
 #define VFPOPDESC_UNUSED_BIT	(24)
 #define VFPOPDESC_UNUSED_MASK	(0xFF << VFPOPDESC_UNUSED_BIT)
 #define VFPOPDESC_OPDESC_MASK	(~(VFPOPDESC_LENGTH_MASK | VFPOPDESC_UNUSED_MASK))
+
+#ifndef __ASSEMBLER__
+#include <linux/types.h>
+
+extern u32 vfp_has_d16_d31;
+#endif
diff --git a/arch/arm/include/asm/vfpmacros.h b/arch/arm/include/asm/vfpmacros.h
index 6a6f1e4..11496a9 100644
--- a/arch/arm/include/asm/vfpmacros.h
+++ b/arch/arm/include/asm/vfpmacros.h
@@ -25,9 +25,9 @@
 #endif
 #ifdef CONFIG_VFPv3
 #if __LINUX_ARM_ARCH__ <= 6
-	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
+	ldr	\tmp, =vfp_has_d16_d31		    @ have VFP regs d16-d31?
 	ldr	\tmp, [\tmp, #0]
-	tst	\tmp, #HWCAP_VFPv3D16
+	cmp	\tmp, #1
 	ldceql	p11, cr0, [\base],#32*4		    @ FLDMIAD \base!, {d16-d31}
 	addne	\base, \base, #32*4		    @ step over unused register space
 #else
@@ -49,9 +49,9 @@
 #endif
 #ifdef CONFIG_VFPv3
 #if __LINUX_ARM_ARCH__ <= 6
-	ldr	\tmp, =elf_hwcap		    @ may not have MVFR regs
+	ldr	\tmp, =vfp_has_d16_d31		    @ have VFP regs d16-d31?
 	ldr	\tmp, [\tmp, #0]
-	tst	\tmp, #HWCAP_VFPv3D16
+	cmp	\tmp, #1
 	stceql	p11, cr0, [\base],#32*4		    @ FSTMIAD \base!, {d16-d31}
 	addne	\base, \base, #32*4		    @ step over unused register space
 #else
diff --git a/arch/arm/vfp/vfpmodule.c b/arch/arm/vfp/vfpmodule.c
index c834b32..929b53b 100644
--- a/arch/arm/vfp/vfpmodule.c
+++ b/arch/arm/vfp/vfpmodule.c
@@ -58,6 +58,12 @@ unsigned int VFP_arch;
 union vfp_state *vfp_current_hw_state[NR_CPUS];
 
 /*
+ * If the VFP on this ARM core has registers d16-d31, vfp_has_d16_d31 will
+ * be set to 1; otherwise, 0.  Only valid after startup.
+ */
+u32 vfp_has_d16_d31;
+
+/*
  * Is 'thread's most up to date state stored in this CPUs hardware?
  * Must be called from non-preemptible context.
  */
@@ -691,6 +697,8 @@ static int __init vfp_init(void)
 		thread_register_notifier(&vfp_notifier_block);
 		vfp_pm_init();
 
+		vfp_has_d16_d31 = 0;
+
 		/*
 		 * We detected VFP, and the support code is
 		 * in place; report VFP support to userspace.
@@ -706,6 +714,8 @@ static int __init vfp_init(void)
 			 */
 			if (((fmrx(MVFR0) & MVFR0_A_SIMD_MASK)) == 1)
 				elf_hwcap |= HWCAP_VFPv3D16;
+			else
+				vfp_has_d16_d31 = 1;
 		}
 #endif
 		/*
-- 
1.7.10.4

^ permalink raw reply related

* [PATCH v4] GPIO: Add support for GPIO on CLPS711X-target platform
From: Alexander Shiyan @ 2012-10-13  4:44 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CACRpkdb7V0ROEQZ3v98wCx3UrD00-___qnRaQ89LLFp_YDKt5Q@mail.gmail.com>

On Fri, 12 Oct 2012 22:55:52 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Tue, Oct 9, 2012 at 6:05 PM, Alexander Shiyan <shc_work@mail.ru> wrote:
> > The CLPS711X CPUs provide some GPIOs for use in the system. This
> > driver provides support for these via gpiolib. Due to platform
> > limitations, driver does not support interrupts, only inputs and
> > outputs.
> >
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> 
> Hey looking good:
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Will you send this through the ARM SoC tree with other
> CLPS711x changes, or should I take it into my GPIO tree?
Yes please, if it possible.
This is a standalone patch and it no depends on other series.

-- 
Alexander Shiyan <shc_work@mail.ru>

^ permalink raw reply

* [PATCH for 3.7] dtb: fix interrupt assignment for ehci/uhci on wm8505
From: Tony Prisk @ 2012-10-13  4:18 UTC (permalink / raw)
  To: linux-arm-kernel

EHCI and UHCI devices in wm8505.dtsi should use IRQ 0 & 1
respectively - not 43 as used on newer models.

Signed-off-by: Tony Prisk <linux@prisktech.co.nz>
---
 arch/arm/boot/dts/wm8505.dtsi |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/wm8505.dtsi b/arch/arm/boot/dts/wm8505.dtsi
index b459691..330f833 100644
--- a/arch/arm/boot/dts/wm8505.dtsi
+++ b/arch/arm/boot/dts/wm8505.dtsi
@@ -71,13 +71,13 @@
 		ehci at d8007100 {
 			compatible = "via,vt8500-ehci";
 			reg = <0xd8007100 0x200>;
-			interrupts = <43>;
+			interrupts = <1>;
 		};
 
 		uhci at d8007300 {
 			compatible = "platform-uhci";
 			reg = <0xd8007300 0x200>;
-			interrupts = <43>;
+			interrupts = <0>;
 		};
 
 		fb at d8050800 {
-- 
1.7.9.5

^ permalink raw reply related

* Testing i2c in 3.6.0-next-20121012 for i.mx233
From: Tim michals @ 2012-10-13  2:48 UTC (permalink / raw)
  To: linux-arm-kernel

Using the?3.6.0-next-20121012 ?to test i2c DMA support on a imx233-Olinuxino-micro?board. ?When using the i2cdetect -r 0 to scan a devices on the i2c bus, ?the following message is displayed on the console:

-- [ ?129.130000] mxs-i2c 80058000.i2c: Failed to get PIO reg. write descriptor.

imx23.dtsi support for i2c:
i2c0_pins_a: i2c0 at 0 {
reg = <0>;
fsl,pinmux-ids = <
0x1171 /* MX23_PAD_LCD_ENABLE__LCD_ENABLE */
0x1181 /* MX23_PAD_LCD_HSYNC__LCD_HSYNC */
>;
fsl,drive-strength = <1>;
fsl,voltage = <1>;
fsl,pull-up = <1>;
};
...
i2c at 80058000 {
#address-cells = <1>;
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?#size-cells = <0>;
compatible = "fsl,imx28-i2c";
reg = <0x80058000 0x2000>;
interrupts = <27 26>;
clock-frequency = <400000>;
fsl,i2c-dma-channel = <3>;
status = "disabled";
};
imx23-olinuxino.dts
i2c0: i2c at 80058000 {
 ? ? ?pinctrl-names = "default";
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?pinctrl-0 = <&i2c0_pins_a>;
? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?status = "okay";
? ? ? ? ? ? ? ? ? ? ? ?};
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121013/8dc310ec/attachment.html>

^ permalink raw reply

* When flush all dcache, why the increasing step is 2?
From: Li Haifeng @ 2012-10-13  0:47 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20121012120946.GB17525@arm.com>

2012/10/12 Catalin Marinas <catalin.marinas@arm.com>

> On Fri, Oct 12, 2012 at 11:51:07AM +0100, Li Haifeng wrote:
> > For flush all data cache, the flush action begins from Level 0, then
> increase
> > cache level by 1.
> >
> > But in function v7_flush_dcache_all, the step is 2. IOW, it will just
> flush
> > 0,2,4,6 level cache.
> > As the following code, the step is stored in r10, and the increase line
> is "add
> >     r10, r10, #2".
>
> That's because the cache level is written in the CSSELR bits 3:1 (so no
> bit 0). When the levels of cache is calculated (r3), it is also 2x.
>

Great, Thanks very much.


>
> --
> Catalin
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121013/125a1cb6/attachment-0001.html>

^ permalink raw reply

* [PATCH 6/6] ARM: EXYNOS5: SATA PHY controller driver
From: Tomasz Figa @ 2012-10-12 22:50 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1349783332-18467-7-git-send-email-vasanthananthan@gmail.com>

Hi,

On Tuesday 09 of October 2012 17:18:52 Vasanth Ananthan wrote:
> This patch adds a platform driver and I2C client driver for SATA PHY
> controller
> 
> Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
> ---
>  drivers/ata/Makefile          |    2 +-
>  drivers/ata/sata_exynos_phy.c |  303
> +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 304
> insertions(+), 1 deletions(-)
>  create mode 100644 drivers/ata/sata_exynos_phy.c
> 
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index 33fb2f6..769d945 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -9,7 +9,7 @@ obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
>  obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
>  obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
>  obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
> -obj-$(CONFIG_SATA_EXYNOS)          += sata_phy.o sata_exynos.o
> libahci.o +obj-$(CONFIG_SATA_EXYNOS)          += sata_phy.o
> sata_exynos_phy.o sata_exynos.o libahci.o
> 
>  # SFF w/ custom DMA
>  obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
> diff --git a/drivers/ata/sata_exynos_phy.c
> b/drivers/ata/sata_exynos_phy.c new file mode 100644
> index 0000000..44861de
> --- /dev/null
> +++ b/drivers/ata/sata_exynos_phy.c
> @@ -0,0 +1,303 @@
> +/*
> + * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
> + *              http://www.samsung.com
> + *
> + * EXYNOS - SATA PHY controller driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as +
> * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/i2c.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +
> +#include <plat/cpu.h>
> +
> +#include <mach/irqs.h>
> +#include <mach/map.h>
> +#include <mach/regs-pmu.h>
> +#include <mach/regs-sata.h>
> +
> +#include "sata_phy.h"
> +
> +#define	SATA_TIME_LIMIT		1000
> +
> +static struct i2c_client *i2c_client;
> +
> +static struct i2c_driver sataphy_i2c_driver;
> +
> +struct exynos_sata_phy {
> +	void __iomem *mmio;
> +	struct resource *mem;
> +	struct clk *clk;
> +};
> +
> +static bool sata_is_reg(void __iomem *base, u32 reg, u32 checkbit, u32
> Status) +{

Please use lowercase for variable names.

> +	if ((readl(base + reg) & checkbit) == Status)
> +		return true;
> +	else
> +		return false;
> +}
> +
> +static bool wait_for_reg_status(void __iomem *base, u32 reg, u32
> checkbit, +				u32 Status)

Same here.

> +{
> +	u16 time_limit_cnt = 0;
> +	while (!sata_is_reg(base, reg, checkbit, Status)) {
> +		if (time_limit_cnt == SATA_TIME_LIMIT)
> +			return false;
> +		udelay(1000);
> +		time_limit_cnt++;
> +	}
> +	return true;
> +}
> +
> +int sataphy_init(struct sata_phy *phy)

static int sataphy_init(struct sata_phy *phy) ?

> +{
> +	int ret;
> +	u32 val;
> +
> +	/* Values to be written to enable 40 bits interface */
> +	u8 buf[] = { 0x3A, 0x0B };
> +
> +	struct exynos_sata_phy *sata_phy;
> +
> +	sata_phy = (struct exynos_sata_phy *)phy->priv_data;
> +
> +	clk_enable(sata_phy->clk);
> +
> +	writel(S5P_PMU_SATA_PHY_CONTROL_EN, EXYNOS5_SATA_PHY_CONTROL);
> +
> +	val = 0;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val |= 0xFF;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val |= LINK_RESET;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val |= RESET_CMN_RST_N;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> +	val &= ~PHCTRLM_REF_RATE;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> +
> +	/* High speed enable for Gen3 */
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> +	val |= PHCTRLM_HIGH_SPEED;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_PHSATA_CTRLM);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_CTRL0);
> +	val |= CTRL0_P0_PHY_CALIBRATED_SEL | CTRL0_P0_PHY_CALIBRATED;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_CTRL0);
> +
> +	writel(SATA_PHY_GENERATION3, sata_phy->mmio + EXYNOS5_SATA_MODE0);
> +
> +	ret = i2c_master_send(i2c_client, buf, sizeof(buf));
> +	if (ret < 0)
> +		return -EINVAL;
> +
> +	/* release cmu reset */
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val &= ~RESET_CMN_RST_N;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	val = readl(sata_phy->mmio + EXYNOS5_SATA_RESET);
> +	val |= RESET_CMN_RST_N;
> +	writel(val, sata_phy->mmio + EXYNOS5_SATA_RESET);
> +
> +	if (wait_for_reg_status(sata_phy->mmio, EXYNOS5_SATA_PHSATA_STATM,
> +				PHSTATM_PLL_LOCKED, 1)) {
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +
> +int sataphy_shutdown(struct sata_phy *phy)

static int sataphy_shutdown(struct sata_phy *phy) ?

> +{
> +
> +	struct exynos_sata_phy *sata_phy;
> +
> +	sata_phy = (struct exynos_sata_phy *)phy->priv_data;
> +
> +	clk_disable(sata_phy->clk);
> +
> +	return 0;
> +}
> +
> +static int sata_i2c_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *i2c_id)
> +{
> +	i2c_client = client;
> +	return 0;
> +}
> +
> +static int __init sata_phy_probe(struct platform_device *pdev)
> +{
> +	struct exynos_sata_phy *sataphy;
> +	struct sata_phy *phy;
> +	struct resource *res;
> +	int ret = 0;
> +
> +	phy = kzalloc(sizeof(struct sata_phy), GFP_KERNEL);

devm_

> +	if (!phy) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	sataphy = kzalloc(sizeof(struct exynos_sata_phy), GFP_KERNEL);

devm_

> +	if (!sataphy) {
> +		dev_err(&pdev->dev, "failed to allocate memory\n");
> +		ret = -ENOMEM;
> +		goto err0;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "Could not find IO resource\n");
> +		ret = -EINVAL;
> +		goto err1;
> +	}
> +
> +	sataphy->mem = request_mem_region(res->start, resource_size(res),
> +								pdev->name);

devm_

> +	if (!sataphy->mem) {
> +		dev_err(&pdev->dev, "Could not request IO resource\n");
> +		ret = -EINVAL;
> +		goto err1;
> +	}
> +
> +	sataphy->mmio =
> +	    ioremap(res->start, resource_size(res));

devm_

> +

nitpick: Unnecessary blank line.

> +	if (!sataphy->mmio) {
> +		dev_err(&pdev->dev, "failed to remap IO\n");
> +		ret = -ENOMEM;
> +		goto err2;
> +	}
> +
> +	sataphy->clk = clk_get(&pdev->dev, "sata_phy");

devm_

> +	if (IS_ERR(sataphy->clk)) {
> +		dev_err(&pdev->dev, "failed to get clk for PHY\n");
> +		ret = PTR_ERR(sataphy->clk);
> +		sataphy->clk = NULL;
> +		goto err3;
> +	}
> +
> +	phy->init = sataphy_init;
> +	phy->shutdown = sataphy_shutdown;
> +	phy->priv_data = (void *)sataphy;
> +	phy->dev = &pdev->dev;
> +
> +	ret = sata_add_phy(phy, SATA_PHY_GENERATION3);
> +	if (ret < 0)
> +		goto err4;

Do you have any warranties that phy callbacks won't get called before i2c 
device probes and sets i2c_client?

> +
> +	ret = i2c_add_driver(&sataphy_i2c_driver);
> +	if (ret < 0)
> +		goto err5;
> +
> +	platform_set_drvdata(pdev, phy);
> +
> +	return ret;
> +
> + err5:
> +	sata_remove_phy(phy);
> +
> + err4:
> +	clk_put(sataphy->clk);
> +
> + err3:
> +	iounmap(sataphy->mmio);
> +
> + err2:
> +	release_resource(sataphy->mem);
> +
> + err1:
> +	kfree(sataphy);
> +
> + err0:
> +	kfree(phy);
> +
> + out:
> +	return ret;
> +}
> +
> +static int sata_phy_remove(struct platform_device *pdev)
> +{
> +	struct sata_phy *phy;
> +	struct exynos_sata_phy *sataphy;
> +
> +	phy = platform_get_drvdata(pdev);
> +
> +	sataphy = (struct exynos_sata_phy *)phy->priv_data;
> +	sata_remove_phy(phy);
> +
> +	iounmap(sataphy->mmio);
> +
> +	release_resource(sataphy->mem);
> +
> +	clk_disable(sataphy->clk);
> +	clk_put(sataphy->clk);
> +
> +	kfree(sataphy);
> +	kfree(phy);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sata_phy_of_match[] = {
> +	{.compatible = "samsung,exynos-sata-phy",},

nitpick: Space after opening and before closing brackets.

> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, sata_phy_of_match);
> +
> +static const struct i2c_device_id phy_i2c_device_match[] = {
> +	{"sataphy", 0},

Same here.

> +};
> +
> +MODULE_DEVICE_TABLE(of, phy_i2c_device_match);
> +
> +static struct platform_driver sata_phy_driver = {
> +	.probe = sata_phy_probe,
> +	.remove = sata_phy_remove,
> +	.driver = {
> +		   .name = "sata-phy",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = sata_phy_of_match,
> +		   },

nitpick: Wrong indentation of closing bracket.

> +};
> +
> +static struct i2c_driver sataphy_i2c_driver = {
> +	.driver = {
> +		   .name = "i2c-phy",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = phy_i2c_device_match,
> +		   },

nitpick: Wrong indentation of closing bracket.

Best regards,
Tomasz Figa

^ permalink raw reply

* [PATCH 5/6] ARM: EYNOS5: SATA controller driver
From: Tomasz Figa @ 2012-10-12 22:45 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1349783332-18467-6-git-send-email-vasanthananthan@gmail.com>

Hi,

On Tuesday 09 of October 2012 17:18:51 Vasanth Ananthan wrote:
> This patch adds a platform driver for SATA controller.
> 
> Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
> ---
>  arch/arm/mach-exynos/include/mach/regs-pmu.h |    3 +
>  drivers/ata/Makefile                         |    2 +-
>  drivers/ata/sata_exynos.c                    |  274
> ++++++++++++++++++++++++++ 3 files changed, 278 insertions(+), 1
> deletions(-)
>  create mode 100644 drivers/ata/sata_exynos.c
> 
> diff --git a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> b/arch/arm/mach-exynos/include/mach/regs-pmu.h index d4e392b..3716ce8
> 100644
> --- a/arch/arm/mach-exynos/include/mach/regs-pmu.h
> +++ b/arch/arm/mach-exynos/include/mach/regs-pmu.h
> @@ -367,4 +367,7 @@
> 
>  #define EXYNOS5_OPTION_USE_RETENTION				(1 << 4)
> 
> +/* Only for EXYNOS5250 */
> +#define EXYNOS5_SATA_PHY_CONTROL		S5P_PMUREG(0x0724)
> +
>  #endif /* __ASM_ARCH_REGS_PMU_H */
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index bf3fd91..33fb2f6 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -9,7 +9,7 @@ obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
>  obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
>  obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
>  obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
> -obj-$(CONFIG_SATA_EXYNOS)          += sata_phy.o libahci.o
> +obj-$(CONFIG_SATA_EXYNOS)          += sata_phy.o sata_exynos.o
> libahci.o
> 
>  # SFF w/ custom DMA
>  obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
> diff --git a/drivers/ata/sata_exynos.c b/drivers/ata/sata_exynos.c
> new file mode 100644
> index 0000000..877039e
> --- /dev/null
> +++ b/drivers/ata/sata_exynos.c
> @@ -0,0 +1,274 @@
> +/*
> + * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
> + *              http://www.samsung.com
> + *
> + * EXYNOS - SATA controller driver
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as +
> * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/gfp.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/interrupt.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/libata.h>
> +#include <linux/ahci_platform.h>
> +#include <linux/clk.h>
> +#include <linux/slab.h>
> +
> +#include "ahci.h"
> +#include "sata_phy.h"
> +
> +#define SATA_FREQ	(66*1000*1000)
> +
> +static const struct ata_port_info ahci_port_info[] = {
> +	[0] = {
> +	       .flags = AHCI_FLAG_COMMON,
> +	       .pio_mask = ATA_PIO4,
> +	       .udma_mask = ATA_UDMA6,
> +	       .port_ops = &ahci_ops,
> +	       },

nitpick: Indentation of closing bracket is incorrect.

> +};
> +
> +static struct scsi_host_template ahci_platform_sht = {
> +	AHCI_SHT("ahci_platform"),
> +};
> +
> +struct exynos_sata {
> +	struct clk *sclk;
> +	struct clk *clk;
> +	struct sata_phy *phy;
> +	int irq;
> +};
> +
> +static int __init exynos_sata_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ata_port_info pi = ahci_port_info[0];
> +	const struct ata_port_info *ppi[] = { &pi, NULL };
> +	struct ahci_host_priv *hpriv;
> +	struct exynos_sata *sata;
> +	struct ata_host *host;
> +	struct resource *mem;
> +	int n_ports, i, ret;
> +
> +	sata = kzalloc(sizeof(*sata), GFP_KERNEL);

Why this one isn't also allocated using devm_kzalloc?

> +	if (!sata) {
> +		dev_err(dev, "can't alloc sata\n");
> +		ret = -ENOMEM;
> +		return ret;

return -ENOMEM;

> +	}
> +
> +	hpriv = devm_kzalloc(dev, sizeof(*hpriv), GFP_KERNEL);
> +	if (!hpriv) {
> +		dev_err(dev, "can't alloc ahci_host_priv\n");
> +		ret = -ENOMEM;
> +		goto err1;
> +	}
> +
> +	hpriv->flags |= (unsigned long)pi.private_data;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!mem) {
> +		dev_err(dev, "no mmio space\n");
> +		ret = -EINVAL;
> +		goto err2;
> +	}
> +
> +	sata->irq = platform_get_irq(pdev, 0);
> +	if (sata->irq <= 0) {
> +		dev_err(dev, "no irq\n");
> +		ret = -EINVAL;
> +		goto err2;
> +	}
> +
> +	hpriv->mmio = devm_ioremap(dev, mem->start, resource_size(mem));
> +	if (!hpriv->mmio) {
> +		dev_err(dev, "can't map %pR\n", mem);
> +		ret = -ENOMEM;
> +		goto err2;
> +	}
> +
> +	sata->sclk = clk_get(dev, "sclk_sata");

devm_clk_get?

> +	if (IS_ERR(sata->sclk)) {
> +		dev_err(dev, "failed to get sclk_sata\n");
> +		ret = PTR_ERR(sata->sclk);
> +		goto err3;
> +	}
> +	clk_enable(sata->sclk);
> +
> +	clk_set_rate(sata->sclk, SATA_FREQ);
> +
> +	sata->clk = clk_get(dev, "sata");

devm_clk_get?

> +	if (IS_ERR(sata->clk)) {
> +		dev_err(dev, "failed to get sata clock\n");
> +		ret = PTR_ERR(sata->clk);
> +		goto err4;
> +	}
> +	clk_enable(sata->clk);
> +
> +	/*  Get a gen 3 PHY controller */
> +
> +	sata->phy = sata_get_phy(SATA_PHY_GENERATION3);
> +	if (IS_ERR(sata->phy)) {
> +		dev_err(dev, "failed to get sata phy\n");
> +		ret = PTR_ERR(sata->clk);
> +		goto err5;
> +	}
> +
> +	/* Initialize the controller */
> +
> +	ret = sata_init_phy(sata->phy);
> +	if (ret)
> +		goto err6;
> +
> +	ahci_save_initial_config(dev, hpriv, 0, 0);
> +
> +	/* prepare host */
> +	if (hpriv->cap & HOST_CAP_NCQ)
> +		pi.flags |= ATA_FLAG_NCQ;
> +
> +	if (hpriv->cap & HOST_CAP_PMP)
> +		pi.flags |= ATA_FLAG_PMP;
> +
> +	ahci_set_em_messages(hpriv, &pi);
> +
> +	/* CAP.NP sometimes indicate the index of the last enabled
> +	 * port, at other times, that of the last possible port, so
> +	 * determining the maximum port number requires looking at
> +	 * both CAP.NP and port_map.
> +	 */
> +	n_ports = max(ahci_nr_ports(hpriv->cap), fls(hpriv->port_map));
> +
> +	host = ata_host_alloc_pinfo(dev, ppi, n_ports);
> +	if (!host) {
> +		ret = -ENOMEM;
> +		goto err7;
> +	}
> +
> +	host->private_data = hpriv;
> +
> +	if (!(hpriv->cap & HOST_CAP_SSS) || ahci_ignore_sss)
> +		host->flags |= ATA_HOST_PARALLEL_SCAN;
> +	else
> +		pr_info(KERN_INFO
> +		       "ahci: SSS flag set, parallel bus scan disabled\n");
> +
> +	if (pi.flags & ATA_FLAG_EM)
> +		ahci_reset_em(host);
> +
> +	for (i = 0; i < host->n_ports; i++) {
> +		struct ata_port *ap = host->ports[i];
> +
> +		ata_port_desc(ap, "mmio %pR", mem);
> +		ata_port_desc(ap, "port 0x%x", 0x100 + ap->port_no * 0x80);
> +
> +		/* set enclosure management message type */
> +		if (ap->flags & ATA_FLAG_EM)
> +			ap->em_message_type = hpriv->em_msg_type;
> +
> +		/* disabled/not-implemented port */
> +		if (!(hpriv->port_map & (1 << i)))
> +			ap->ops = &ata_dummy_port_ops;
> +	}
> +
> +	ret = ahci_reset_controller(host);
> +	if (ret)
> +		goto err7;
> +
> +	ahci_init_controller(host);
> +	ahci_print_info(host, "platform");
> +
> +	ret = ata_host_activate(host, sata->irq, ahci_interrupt, 
IRQF_SHARED,
> +				&ahci_platform_sht);
> +	if (ret)
> +		goto err7;
> +
> +	platform_set_drvdata(pdev, sata);
> +
> +	return 0;
> +
> + err7:
> +	sata_shutdown_phy(sata->phy);
> +
> + err6:
> +	sata_put_phy(sata->phy);
> +
> + err5:
> +	clk_disable(sata->clk);
> +	clk_put(sata->clk);
> +
> + err4:
> +	clk_disable(sata->sclk);
> +	clk_put(sata->sclk);
> +
> + err3:
> +	iounmap(hpriv->mmio);
> +
> + err2:
> +	kfree(hpriv);
> +
> + err1:
> +	kfree(sata);
> +
> +	return ret;
> +}
> +
> +static int __devexit exynos_sata_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct ata_host *host = dev_get_drvdata(dev);
> +	struct exynos_sata *sata = platform_get_drvdata(pdev);
> +	ata_host_detach(host);
> +
> +	sata_shutdown_phy(sata->phy);
> +	sata_put_phy(sata->phy);
> +
> +	clk_disable(sata->sclk);
> +	clk_put(sata->sclk);
> +
> +	clk_disable(sata->clk);
> +	clk_put(sata->clk);
> +
> +	kfree(sata);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ahci_of_match[] = {
> +	{.compatible = "samsung,exynos-sata-ahci",},

nitpick: Space after opening and before closing bracket. 

> +};
> +
> +MODULE_DEVICE_TABLE(of, ahci_of_match);
> +
> +static struct platform_driver exynos_sata_driver = {
> +	.remove = exynos_sata_remove,
> +	.driver = {
> +		   .name = "ahci-sata",
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = ahci_of_match,
> +		   },

nitpick: Incorrect indentation.

> +};
> +
> +static int __init exynos_sata_init(void)
> +{
> +	return platform_driver_probe(&exynos_sata_driver, 
exynos_sata_probe);
> +}
> +
> +module_init(exynos_sata_init);
> +
> +static void __exit exynos_sata_exit(void)
> +{
> +	platform_driver_unregister(&exynos_sata_driver);
> +}
> +
> +module_exit(exynos_sata_exit);

module_platform_driver(exynos_sata_driver);

Best regards,
Tomasz Figa

^ permalink raw reply

* [PATCH 4/6] ARM: S3C2410: I2C driver polling mode support
From: Tomasz Figa @ 2012-10-12 22:36 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1349783332-18467-5-git-send-email-vasanthananthan@gmail.com>

Hi,

On Tuesday 09 of October 2012 17:18:50 Vasanth Ananthan wrote:
> This patch adds polling mode support for i2c s3c-2410 driver.
> The I2C_SATAPHY controller lacks an interrupt line but the s3c-2410
> driver is interrupt driven. Hence this support is required for
> functioning of the I2C_SATAPHY controller.
> 
> Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
> ---
>  drivers/i2c/busses/i2c-s3c2410.c |   84
> +++++++++++++++++++++++++++++--------- 1 files changed, 65
> insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c
> b/drivers/i2c/busses/i2c-s3c2410.c index 5ae3b02..699b530 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -48,6 +48,7 @@
>  #define QUIRK_S3C2440		(1 << 0)
>  #define QUIRK_HDMIPHY		(1 << 1)
>  #define QUIRK_NO_GPIO		(1 << 2)
> +#define QUIRK_SATAPHY		(1 << 3)
> 
>  /* i2c controller state */
>  enum s3c24xx_i2c_state {
> @@ -102,10 +103,14 @@ static struct platform_device_id
> s3c24xx_driver_ids[] = { };
>  MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
> 
> +static int i2c_s3c_irq_nextbyte(struct s3c24xx_i2c *i2c, unsigned long
> iicstat); +
>  #ifdef CONFIG_OF
>  static const struct of_device_id s3c24xx_i2c_match[] = {
>  	{ .compatible = "samsung,s3c2410-i2c", .data = (void *)0 },
>  	{ .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440
> }, +	{ .compatible = "samsung,s3c2440-sataphy-i2c",
> +	  .data = (void *)(QUIRK_S3C2440|QUIRK_SATAPHY|QUIRK_NO_GPIO) },

nitpick: Please insert spaces around bitwise OR operators.

>  	{ .compatible = "samsung,s3c2440-hdmiphy-i2c",
>  	  .data = (void *)(QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO) },
>  	{},
> @@ -146,7 +151,8 @@ static inline void
> s3c24xx_i2c_master_complete(struct s3c24xx_i2c *i2c, int ret) if (ret)
>  		i2c->msg_idx = ret;
> 
> -	wake_up(&i2c->wait);
> +	if (!(i2c->quirks & QUIRK_SATAPHY))
> +		wake_up(&i2c->wait);
>  }
> 
>  static inline void s3c24xx_i2c_disable_ack(struct s3c24xx_i2c *i2c)
> @@ -184,6 +190,21 @@ static inline void s3c24xx_i2c_enable_irq(struct
> s3c24xx_i2c *i2c) }
> 
> 
> +static bool is_ack(struct s3c24xx_i2c *i2c)
> +{
> +	u32 time_out = i2c->tx_setup;

nitpick: Separate local variable declaration from code with a blank line.

> +	while (--time_out) {
> +		if (readl(i2c->regs + S3C2410_IICCON)
> +			& S3C2410_IICCON_IRQPEND) {
> +			if (!(readl(i2c->regs + S3C2410_IICSTAT)
> +				& S3C2410_IICSTAT_LASTBIT))
> +				return true;
> +		}
> +		udelay(time_out);
> +	}

nitpick: Here a blank line would be fine.

> +	return false;
> +}
> +
>  /* s3c24xx_i2c_message_start
>   *
>   * put the start of a message onto the bus
> @@ -227,6 +248,21 @@ static void s3c24xx_i2c_message_start(struct
> s3c24xx_i2c *i2c,
> 
>  	stat |= S3C2410_IICSTAT_START;
>  	writel(stat, i2c->regs + S3C2410_IICSTAT);
> +
> +	if (i2c->quirks & QUIRK_SATAPHY) {
> +

nitpick: Unnecessary blank line.

> +		while ((i2c->msg_num != 0) && is_ack(i2c)) {
> +

Same here.

> +			i2c_s3c_irq_nextbyte(i2c, stat);
> +
> +			stat = readl(i2c->regs + S3C2410_IICSTAT);
> +			if (stat & S3C2410_IICSTAT_ARBITR)
> +				dev_err(i2c->dev, "deal with arbitration loss\n");
> +

Same here.

> +		}
> +
Same here.

> +	}
> +

Same here.

>  }
>

Best regards,
Tomasz Figa

^ permalink raw reply

* [PATCH 3/6] DRIVERS: ATA: SATA PHY utility framework
From: Tomasz Figa @ 2012-10-12 22:30 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1349783332-18467-4-git-send-email-vasanthananthan@gmail.com>

Hi Vasanth, 

On Tuesday 09 of October 2012 17:18:49 Vasanth Ananthan wrote:
> This patch adds SATA PHY utility framework APIs. The framework acts as
> an interface between the SATA device and the PHY device. The SATA PHY
> device registers itself with the framework through the APIs provided
> and the SATA device finds and requests for an appropriate PHY device.
> 
> Signed-off-by: Vasanth Ananthan <vasanth.a@samsung.com>
> ---
>  drivers/ata/Kconfig    |    9 ++++
>  drivers/ata/Makefile   |    1 +
>  drivers/ata/sata_phy.c |   99
> ++++++++++++++++++++++++++++++++++++++++++++++++ drivers/ata/sata_phy.h
> |   44 +++++++++++++++++++++
>  4 files changed, 153 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/ata/sata_phy.c
>  create mode 100644 drivers/ata/sata_phy.h
> 
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index 27cecd3..0344b78 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -83,6 +83,15 @@ config SATA_AHCI_PLATFORM
> 
>  	  If unsure, say N.
> 
> +config SATA_EXYNOS
> +	bool "Exynos SATA AHCI support"
> +	depends on I2C_S3C2410
> +	help
> +          This option enables support for Exynos AHCI Serial ATA
> +          controllers.
> +
> +          If unsure, say N.
> +
>  config SATA_FSL
>  	tristate "Freescale 3.0Gbps SATA support"
>  	depends on FSL_SOC
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index a454a13..bf3fd91 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_SATA_FSL)		+= sata_fsl.o
>  obj-$(CONFIG_SATA_INIC162X)	+= sata_inic162x.o
>  obj-$(CONFIG_SATA_SIL24)	+= sata_sil24.o
>  obj-$(CONFIG_SATA_DWC)		+= sata_dwc_460ex.o
> +obj-$(CONFIG_SATA_EXYNOS)          += sata_phy.o libahci.o


If the framework introduced by this patch is supposed to be generic, maybe 
a new Kconfig entry should be created for it, like CONFIG_SATA_PHY, which 
would be selected by any drivers using it?

> 
>  # SFF w/ custom DMA
>  obj-$(CONFIG_PDC_ADMA)		+= pdc_adma.o
> diff --git a/drivers/ata/sata_phy.c b/drivers/ata/sata_phy.c
> new file mode 100644
> index 0000000..dbb4aa3
> --- /dev/null
> +++ b/drivers/ata/sata_phy.c
> @@ -0,0 +1,99 @@
> +/*
> + * Copyright (c) 2010-2012 Samsung Electronics Co., Ltd.
> + *              http://www.samsung.com
> + *
> + * EXYNOS - SATA utility framework.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as +
> * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/export.h>
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/list.h>
> +#include "sata_phy.h"
> +
> +static LIST_HEAD(phy_list);
> +static DEFINE_SPINLOCK(phy_lock);
> +
> +struct sata_phy *sata_get_phy(enum sata_phy_type type)
> +{
> +	struct sata_phy *x = NULL;
> +	unsigned long flag;
> +
> +	spin_lock_irqsave(&phy_lock, flag);
> +
> +	list_for_each_entry(x, &phy_list, head) {
> +		if (x->type == type) {
> +			get_device(x->dev);
> +			break;
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&phy_lock, flag);
> +	return x;
> +}
> +EXPORT_SYMBOL(sata_get_phy);
> +
> +int sata_add_phy(struct sata_phy *phy, enum sata_phy_type type)
> +{
> +	unsigned long flag;
> +	unsigned int ret = -EINVAL;
> +	struct sata_phy *x;

If you need to handle the situation when phy is NULL here, then why not 
to:

	if (!phy)
		return -EINVAL;

and then make the code below unconditional?

> +
> +	spin_lock_irqsave(&phy_lock, flag);
> +
> +	if (phy) {
> +
> +		list_for_each_entry(x, &phy_list, head) {
> +			if (x->type == type) {
> +				dev_err(phy->dev, "transceiver type already 
exists\n");
> +				goto out;
> +			}
> +		}
> +		phy->type = type;
> +		list_add_tail(&phy->head, &phy_list);
> +		ret = 0;
> +	}
> +
> + out:
> +	spin_unlock_irqrestore(&phy_lock, flag);
> +	return ret;
> +}
> +EXPORT_SYMBOL(sata_add_phy);
> +
> +void sata_remove_phy(struct sata_phy *phy)
> +{
> +	unsigned long flag;
> +	struct sata_phy *x;

Same here.

Best regards,
Tomasz Figa

^ permalink raw reply

* [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
From: Tony Lindgren @ 2012-10-12 21:51 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <87lifbwhwd.fsf@deeprootsystems.com>

* Kevin Hilman <khilman@deeprootsystems.com> [121012 13:34]:
>
> I'm not conviced (yet) that a mismatch is the root cause.  Yes, that's
> what the author of $SUBJECT patch assumed and stated, but I'm not
> pursuaded.  
> 
> If it's an improperly configured mux issue, then the UART will break
> whenever the device is actually omap_device_enable'd, whether in the
> driver or in the bus layer.

I tried booting n800 here with CONFIG_OMAP_MUX disabled, and no
change. Serial console output stops right when the console initializes.

Regards,

Tony

^ permalink raw reply

* [PATCH] mmc: mmci: Fixup and cleanup code for DMA handling
From: Linus Walleij @ 2012-10-12 21:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1350056012-18625-1-git-send-email-ulf.hansson@stericsson.com>

On Fri, Oct 12, 2012 at 5:33 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:

> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> The cookie is now used to indicate if dma_unmap_sg shall be
> done in post_request. At DMA errors, the DMA job is immediately
> not only terminated but also unmapped. To indicate that this
> has been done the cookie is reset to zero. post_request will
> thus only do dma_umap_sg for requests which has a cookie not set
> to zero.
>
> Some corresponding duplicated code could then be removed and
> moreover some corrections at DMA errors for terminating the same
> DMA job twice has also been fixed.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Per Forlin <per.forlin@stericsson.com>

It looks like it's both factoring out code and also adding some unmapping
in hithereto unhandled cases, correct? It looks OK to me now atleast so
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply

* [PATCH 2/6] ARM: OMAP3/4: iommu: adapt to runtime pm
From: Felipe Contreras @ 2012-10-12 21:25 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CALLhW=5k=tAXo_5ri2WgetRgju9rC_Wm-o9-pfHSNqpe6FU5qQ@mail.gmail.com>

On Fri, Oct 12, 2012 at 7:46 PM, Omar Ramirez Luna <omar.luna@linaro.org> wrote:
> On 12 October 2012 02:48, Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> I already made most of these comments, but here they go again.
>
> I replied to all, but here it goes again:

Mostly, but not all :)

>>> @@ -142,11 +142,10 @@ static int iommu_enable(struct omap_iommu *obj)
>>>                 }
>>>         }
>>>
>>> -       clk_enable(obj->clk);
>>> +       pm_runtime_get_sync(obj->dev);
>>>
>>>         err = arch_iommu->enable(obj);
>>>
>>> -       clk_disable(obj->clk);
>>
>> The device will never go to sleep, until iommu_disable is called.
>> clk_enable -> pm_runtime_get_sync, clk_disable pm_runtime_put.
>
> Which is what you want... why would you want your iommu to be disabled
> if the client of that iommu could request a translation?

That's the whole point of *dynamic* pm; _when_ the client wants to
request a translation, _then_ the device is waken up, which is what I
believe the code currently does.

After your patch, even if I don't use the camera, or the DSP, the
iommu devices will be enabled, and will be consuming energy *all the
time*. Which I don't think is what we want.

I'm not saying I have a solution, I'm simply saying that's what's
going to happen if I'm correct.

> Remember that these iommus, sit along of other processors not on the
> main processor side. So, this code should enable it for the other
> processor to use, and there is no point where the processor can say
> "I'm not using it, shut it down" or "I'm using it, turn it on" in the
> middle of execution, other than suspend/resume and if supported,
> autosuspend.

I understand, but perhaps there should be?

>>> @@ -288,7 +285,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e)
>>>         if (!obj || !obj->nr_tlb_entries || !e)
>>>                 return -EINVAL;
>>>
>>> -       clk_enable(obj->clk);
>>> +       pm_runtime_get_sync(obj->dev);
>>>
>>>         iotlb_lock_get(obj, &l);
>>>         if (l.base == obj->nr_tlb_entries) {
>>> @@ -318,7 +315,7 @@ static int load_iotlb_entry(struct omap_iommu *obj, struct iotlb_entry *e)
>>>
>>>         cr = iotlb_alloc_cr(obj, e);
>>>         if (IS_ERR(cr)) {
>>> -               clk_disable(obj->clk);
>>> +               pm_runtime_put_sync(obj->dev);
>>>                 return PTR_ERR(cr);
>>>         }
>>
>> If I'm correct, the above pm_runtime_get/put are redundant, because
>> the count can't possibly reach 0 because of the reason I explained
>> before.
>>
>> The same for all the cases below.
>
> You can access this paths through debugfs, some of them doesn't work
> if the module is not enabled first, but in future if you just want to
> idle the iommu withouth freeing, these are needed to debug.
>
> BTW, the next patch in the series: ARM: OMAP: iommu: pm runtime save
> and restore context, let's you do a pm_runtime_[enable|put] through
> save/restore ctx functions, which is just for compatibility on how isp
> code uses the save and restore code.

All right, it has been some time since I've touched pm code, so if you say so.

>>> @@ -1009,7 +1001,8 @@ static int __devexit omap_iommu_remove(struct platform_device *pdev)
>>>         release_mem_region(res->start, resource_size(res));
>>>         iounmap(obj->regbase);
>>>
>>> -       clk_put(obj->clk);
>>> +       pm_runtime_disable(obj->dev);
>>
>> This will turn on the device unnecessarily, wasting power, and there's
>> no need for that, kfree will take care of that without resuming.
>
> Left aside the aesthetics of having balanced calls, the device will be
> enabled if there was a pending resume to be executed, otherwise it
> won't, kfree won't increment the disable_depth counter and I don't
> think that freeing the pointer is enough reason to ignore
> pm_runtime_disable.

You are doing __pm_runtime_disable(dev, true), kfree will do
__pm_runtime_disable(dev, false), which is what we want. Both will
decrement the disable_depth.

But at least you agree that there's a chance that the device will be waken up.

>> Also, I still think that something like this is needed:
> ...
>> +static struct clk cam_fck = {
>> +       .name           = "cam_fck",
>> +       .ops            = &clkops_omap2_iclk_dflt,
>> +       .parent         = &l3_ick,
>> +       .enable_reg     = OMAP_CM_REGADDR(OMAP3430_CAM_MOD, CM_ICLKEN),
>
> a cam_fck name to enable the ick?

Yeap, according to the TRM. Take a look at 12.3 Camera ISP Integration
Fig 12-50.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply

* [PATCH] [ARM] Use AT() in the linker script to create correct program headers
From: Jason Gunthorpe @ 2012-10-12 21:24 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20121010095544.GB2131@linaro.org>

On Wed, Oct 10, 2012 at 10:55:44AM +0100, Dave Martin wrote:

> As a side comment, some things such as MAC addresses can be probed and
> set from userspace after kernel boot, assuming that you have a way
> to fetch the device description blob from userspace.  If it's accessible
> via MTD I'm guessing you may have some chance of doing that.

Yes, there is a whole wack of other data in this structure (serial
numbers, etc) that are captured and used by userspace, so access is
always made possible.

Setting the MAC via userspace is interesting, I will bear that in
mind. However, we have several development-support boot scenarios that
enable the ethernet before the userspace code to fetch the data
structure is loaded - one of interest has the kernel's initramfs
perform a NBD mount of the actual application filesystem. So having
the kernel tend to this is very convenient.
 
> However, I worry that if you have to run hardware-dependent code in
> order to fetch or generate parts of the DT, then we have a chicken-
> and-egg problem with no guaranteed solution with the frameworks that
> exist today -- although I am not familiar with how DT gets used on
> all other arches, so you might have counterexamples I'm not aware of.

Certainly, in some cases, (like what I have done) such kernels are
non-generic and could only work with the hardware they are intended
for.

However, if you assume the bootloader provides a dtb (even an
incomplete one would do) then fixups/replacements/whatever could be
run based on tags in the dtb without creating a chicken-and-egg
problem.

> At least in ARM-land, the DT is inherently monolithic and static: the
> DT is not expected to change once you enter the kernel.

Sort of, it is the same on all arches, the DT block is expected to
remain valid, but all the of_node pointers go directly into the block
so you can alter values but not the structure without breaking any
invariants. As an example our DTBs all have mac address blocks, with a
0 value. The fixup simply overwrites with the correct value before the
ethernet driver reads from it.

> > I'm not sure there is a great interest in this? What are other folks
> > working on production embedded stuff doing? I suppose that will be
> > more clear as device tree is rolled out.
> 
> For now, the architecturally simplest solution still seems to me to be
> to write your own boot shim which customises the DT before booting the 
> kernel. As discussed, this can continue to look like a simple ELF image
> from the bootloader's point of view -- but I appreciate that it will
> involve effort and some duplication of some low-level driver components.

I admit, I am a bit conflicted. Viewing from the kernel perspective, I
definitely think this kind of very low level extremely board specific
stuff should not be in the kernel. That could be a maintenance
nightmare. However, as an OEM, I need the smallest, simplest boot
loader possible so I want to push this into the kernel to lighten my
development load.

> Your problem is unlikely to affect people outside the embedded space
> too much, but it doesn't sound likely to be unique.

Agreed..

This thread has left the original topic, what do you think I should do
with the linker patch?

Jason

^ permalink raw reply

* [PATCH] mmc: mmci: Support non-power-of-two block sizes for ux500v2 variant
From: Linus Walleij @ 2012-10-12 21:22 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1350050522-8852-1-git-send-email-ulf.hansson@stericsson.com>

On Fri, Oct 12, 2012 at 4:02 PM, Ulf Hansson <ulf.hansson@stericsson.com> wrote:

> From: Ulf Hansson <ulf.hansson@linaro.org>
>
> For the ux500v2 variant non power of two block sizes are supported.
> This will make it possible to decrease data overhead for SDIO
> transfers. Although we need to put some constraints to the alignment
> of the buffers when enabling this feature.
>
> Buffers must be 4 bytes aligned due to restrictions that the PL18x
> FIFO accesses must be done in a 4 byte aligned manner. Moreover we
> need to enable DMA_REQCTL for SDIO to support write of non 32 bytes
> aligned sg element lengths. In PIO mode any buffer length can be
> handled as long as the buffer address is 4 byte aligned.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Per Forlin <per.forlin@stericsson.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

I finally understand how this works now.

Bonus for comments like this:

> +/*
> + * DMA request control is required for write
> + * if transfer size is not 32 byte aligned.
> + * DMA request control is also needed if the total
> + * transfer size is 32 byte aligned but any of the
> + * sg element lengths are not aligned with 32 byte.
> + */
>  #define MCI_ST_DPSM_DMAREQCTL  (1 << 12)

Which make you understand what is actually happening.

Yours,
Linus Walleij

^ permalink raw reply

* [PATCH 4/6] ARM: ux500: Add UART support to the u9540 Device Tree
From: Linus Walleij @ 2012-10-12 21:15 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20121011180612.GY12801@game.jcrosoft.org>

On Thu, Oct 11, 2012 at 8:06 PM, Jean-Christophe PLAGNIOL-VILLARD
<plagnioj@jcrosoft.com> wrote:
> On 10:22 Thu 11 Oct     , Lee Jones wrote:

>> The board could actually be called either ccu9540, or just
>> u9540. I picked one and went with it. The full name is the
>> u9540 development board.
>
> so use ccu9540 as u9540 is the soc name so if we use u9540 as compatible we
> expect to mach the soc and not a board

I basically agree.

However the SoC (digital ASIC) is named DB9540.

U9540 is the entire chipset (DB9540, AB9540 etc) a variant
of the NovaThor series.

Yours,
Linus Walleij

^ permalink raw reply

* [PATCH 2/2] input: gpio-keys: Add runtime support
From: Linus Walleij @ 2012-10-12 21:09 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <CAM=Q2ctwTBAvKs401GgrGfz9NvaLspquje-6YfnuS5TZYeqY4Q@mail.gmail.com>

On Thu, Oct 11, 2012 at 4:22 PM, Shubhrajyoti Datta
<omaplinuxkernel@gmail.com> wrote:

>> @@ -526,6 +527,7 @@ static int gpio_keys_open(struct input_dev *input)
>>  {
>>         struct gpio_keys_drvdata *ddata = input_get_drvdata(input);
>>
>> +       pm_runtime_get_sync(input->dev.parent);
>
> I am not an expert of the runtime.
>
> However would be grateful if you explain me what it actually do.

This increase the reference count of the runtime status container
for the device. _sync makes sure it happens now.

Consult:
Documentation/power/runtime_pm.txt

> Also I did not see any runtime suspend/ resume handlers populated.

It is not necessary to handle the power state at the driver level,
it can just as well be handled by the voltage/power domain,
or at the class, type or bus level.

But the individual driver has to notify the system upward if it
needs to be powered on or when it may or must be relaxed.

Yours,
Linus Walleij

^ permalink raw reply

* [PATCH 7/7] ARM: tegra30: cpuidle: add LP2 driver for CPU0
From: Stephen Warren @ 2012-10-12 21:04 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1350025676.20241.82.camel@jlo-ubuntu-64.nvidia.com>

On 10/12/2012 01:07 AM, Joseph Lo wrote:
> On Fri, 2012-10-12 at 00:37 +0800, Stephen Warren wrote:
>> On 10/11/2012 05:24 AM, Joseph Lo wrote:
>>> On Wed, 2012-10-10 at 06:49 +0800, Stephen Warren wrote:
>>>> On 10/08/2012 04:26 AM, Joseph Lo wrote:
>>>>> The cpuidle LP2 is a power gating idle mode. It support power gating
>>>>> vdd_cpu rail after all cpu cores in LP2. For Tegra30, the CPU0 must
>>>>> be last one to go into LP2. We need to take care and make sure whole
>>>>> secondary CPUs were in LP2 by checking CPU and power gate status.
>>>>> After that, the CPU0 can go into LP2 safely. Then power gating the
>>>>> CPU rail.
>>>>
>>>>> diff --git a/arch/arm/mach-tegra/cpuidle-tegra30.c b/arch/arm/mach-tegra/cpuidle-tegra30.c
>>>>
>>>>> +static bool tegra30_idle_enter_lp2_cpu_0(struct cpuidle_device *dev,
>>>>> +					 struct cpuidle_driver *drv,
>>>>> +					 int index)
>>>>> +{
>>>>> +	struct cpuidle_state *state = &drv->states[index];
>>>>> +	u32 cpu_on_time = state->exit_latency;
>>>>> +	u32 cpu_off_time = state->target_residency - state->exit_latency;
>>>>> +
>>>>> +	if (num_online_cpus() > 1 && !tegra_cpu_rail_off_ready()) {
>>>>
>>>> Should that be || not &&?
>>>>
>>>> Isn't the "num_online_cpus() > 1" condition effectively checked at the
>>>> call site, i.e. in tegra30_idle_lp2() below via the if (last_cpu) check?
>>>>
>>>
>>> Should be "&&" here.
>>> Because we need to check if there are still multi CPUs online, then we
>>> need to make sure all the secondary CPUs be power gated first. After all
>>> the secondary CPUs been power gated, the CPU0 could go into LP2 and the
>>> CPU rail could be shut off.
>>> If all the secondary CPUs been hot plugged, then the "num_online_cpus()
>>>> 1" would be always false. Then the CPU0 can go into LP2 directly.
>>>
>>> So it was used to check are there multi cpus online or not? It's
>>> difference with the last_cpu check below. The last_cpu was used to check
>>> all the CPUs were in LP2 process or not. If the CPU0 is the last one
>>> went into LP2 process, then it would be true.
>>>
>>> So the point here is. We can avoid to check the power status of the
>>> secodarys CPUs if they be unplugged.
>>
>> OK, so this condition is about ignoring the result of
>> tegra_cpu_rail_off_ready() if there is only 1 CPU online. That makes
>> sense, since we know in that case there cannot be any other CPUs to
>> check if they're in LP2 or not.
>>
>> But what about the case where 2 CPUs are online and 2 offline. In that
>> case, num_online_cpus() > 1, so the call to tegra_cpu_rail_off_ready()
>> is skipped.

Uggh. I'm not thinking straight, so what I said there was backwards.

>> b) If CPUn can't trigger rail-gating, then when CPUn is the last to
>> enter LP2 of the whole complex, it needs to IPI to CPU0 to tell it to
>> rail-gate, and simply power-gate itself. I believe this IPI interaction
>> is exactly what coupled cpuidle is about, isn't it?
> 
> Yes, indeed. Actually, I had tried the coupled cpuidle on Tegra20. I
> knew it a lot. But I met issues when porting it. It looks like a race
> condition and becomes a dead lock caused by IPI missing. Anyway, we can
> talk about it more detail when I try to upstream the coupled cpuidle
> support for Tegra later.

Hmm. That sounds a little churny. Why can't we just use coupled cpuidle
right from the start if the plan is to use it eventually anyway? From
other comments, it sounds like you even already have the code basically
working, but just need to iron out a bug or two?

^ permalink raw reply

* [PATCH v4] GPIO: Add support for GPIO on CLPS711X-target platform
From: Linus Walleij @ 2012-10-12 20:55 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1349798756-24777-1-git-send-email-shc_work@mail.ru>

On Tue, Oct 9, 2012 at 6:05 PM, Alexander Shiyan <shc_work@mail.ru> wrote:

> The CLPS711X CPUs provide some GPIOs for use in the system. This
> driver provides support for these via gpiolib. Due to platform
> limitations, driver does not support interrupts, only inputs and
> outputs.
>
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>

Hey looking good:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Will you send this through the ARM SoC tree with other
CLPS711x changes, or should I take it into my GPIO tree?

Yours,
Linus Walleij

^ permalink raw reply

* [PATCH 7/7] ARM: tegra30: cpuidle: add LP2 driver for CPU0
From: Colin Cross @ 2012-10-12 20:50 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20121012083017.GB4962@S2101-09.ap.freescale.net>

On Fri, Oct 12, 2012 at 1:30 AM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Fri, Oct 12, 2012 at 04:24:24PM +0800, Joseph Lo wrote:
>> On Fri, 2012-10-12 at 15:54 +0800, Shawn Guo wrote:
>> > On Thu, Oct 11, 2012 at 09:48:45AM -0700, Colin Cross wrote:
>> > > As is, coupled cpuidle will work on Tegra30, but it will unnecessarily
>> > > wake up the secondary cpus during the transitions to off and back on
>> > > again.  Those cpus will immediately go back to single-cpu LP2,
>> >
>> > I'm sure coupled cpuidle will work like that.  We have the following
>> > code at the end of  cpuidle_enter_state_coupled() to wait until all
>> > coupled cpus have exited idle.
>> >
>> >         while (!cpuidle_coupled_no_cpus_ready(coupled))
>> >                 cpu_relax();
>> >
>> > The cpu woken up during the transitions will just loop there until all
>> > other 3 cpus exit from idle function.
>> >
>>
>> Did this a good idea if the CPU was been woken up from an interrupt but
>> it still needed to wait all other CPUs been woken up then it could
>> handle the interrupt?
>>
> This is how coupled cpuidle gets implemented right now.  And that's
> why I see RCU stall warning reported on that cpu when I tried coupled
> cpuidle on imx6q (CA9 Quad).

Current coupled cpuidle requires all cpus to wake up together and go
back to the idle governor to select a new state, because that's what
all available ARM cpus did when I wrote it.  You should be able to
implement coupled idle on a cpu that does not need to wake all the
cpus if you wake them anyways, and then later you can optimize it to
avoid the extra wakeups when the extension to coupled cpuidle that I
discussed previously gets implemented.

^ permalink raw reply

* [PATCH 7/7] ARM: tegra30: cpuidle: add LP2 driver for CPU0
From: Colin Cross @ 2012-10-12 20:46 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1350030264.15495.14.camel@jlo-ubuntu-64.nvidia.com>

On Fri, Oct 12, 2012 at 1:24 AM, Joseph Lo <josephl@nvidia.com> wrote:
> On Fri, 2012-10-12 at 15:54 +0800, Shawn Guo wrote:
>> On Thu, Oct 11, 2012 at 09:48:45AM -0700, Colin Cross wrote:
>> > As is, coupled cpuidle will work on Tegra30, but it will unnecessarily
>> > wake up the secondary cpus during the transitions to off and back on
>> > again.  Those cpus will immediately go back to single-cpu LP2,
>>
>> I'm sure coupled cpuidle will work like that.  We have the following
>> code at the end of  cpuidle_enter_state_coupled() to wait until all
>> coupled cpus have exited idle.
>>
>>         while (!cpuidle_coupled_no_cpus_ready(coupled))
>>                 cpu_relax();
>>
>> The cpu woken up during the transitions will just loop there until all
>> other 3 cpus exit from idle function.
>>
>
> Did this a good idea if the CPU was been woken up from an interrupt but
> it still needed to wait all other CPUs been woken up then it could
> handle the interrupt?

Each cpu enables its local interrupts between when it clears it's
ready flag and when it enters the loop above, so it will already be
handling interrupts.

^ permalink raw reply

* [RFT/PATCH] serial: omap: prevent resume if device is not suspended.
From: Kevin Hilman @ 2012-10-12 20:32 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <20121012185426.GS28061@n2100.arm.linux.org.uk>

Russell King - ARM Linux <linux@arm.linux.org.uk> writes:

> On Fri, Oct 12, 2012 at 10:59:22AM -0700, Kevin Hilman wrote:
>> Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
>> 
>> > On Fri, Oct 12, 2012 at 09:35:54AM -0700, Kevin Hilman wrote:
>> >> Sourav <sourav.poddar@ti.com> writes:
>> >> > diff --git a/drivers/tty/serial/omap-serial.c
>> >> > b/drivers/tty/serial/omap-serial.c
>> >> > index 6ede6fd..3fbc7f7 100644
>> >> > --- a/drivers/tty/serial/omap-serial.c
>> >> > +++ b/drivers/tty/serial/omap-serial.c
>> >> > @@ -1414,6 +1414,7 @@ static int __devinit serial_omap_probe(struct
>> >> > platform_device *pdev)
>> >> >         INIT_WORK(&up->qos_work, serial_omap_uart_qos_work);
>> >> >
>> >> >         platform_set_drvdata(pdev, up);
>> >> > +       pm_runtime_set_active(&pdev->dev);
>> >> 
>> >> NAK.
>> >> 
>> >> This will obviously break platforms where the UARTs are not active
>> >> before driver loads.
>> >
>> > I thought I had proposed a solution for this issue, which was this
>> > sequence:
>> >
>> >         omap_device_enable(dev);                                                
>> >         pm_runtime_set_active(dev);                                             
>> >         pm_runtime_enable(dev);                                                 
>> >
>> > Yes, I can understand people not liking the omap_device_enable()
>> > there, but I also notice that the email suggesting that never got a
>> > reply either - not even a "I tried this and it doesn't work" or "it
>> > does work".
>> 
>> Yes, that solution would work (though I didn't actually try it.)
>> 
>> However, we can't use omap_device_enable() in the driver.  We're trying
>> to clean all the drivers of OMAP-specific APIs.  That being said,
>> something similar could be done in the device/board init code to ensure
>> the UART HW is in the state that the driver is expecting it, but again,
>> that would just mask the real problem which is that a (re)init of the
>> console UART on 2420/n800 is causing output to disappear.
>
> See my more expansive suggestion just posted.
>
> Whatever way, this discrepancy between runtime PM state and actual device
> state is what is biting you, and it is that which needs fixing.  

I'm not conviced (yet) that a mismatch is the root cause.  Yes, that's
what the author of $SUBJECT patch assumed and stated, but I'm not
pursuaded.  

If it's an improperly configured mux issue, then the UART will break
whenever the device is actually omap_device_enable'd, whether in the
driver or in the bus layer.

Kevin

^ permalink raw reply

* [PATCH] ARM: dts: compile Integrator device trees
From: Linus Walleij @ 2012-10-12 20:22 UTC (permalink / raw)
  To: linux-arm-kernel

This makes sure that the ARM Integrator device trees get compiled
during build.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 arch/arm/boot/dts/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index c1ce813..f37cf9f 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -25,6 +25,8 @@ dtb-$(CONFIG_ARCH_EXYNOS) += exynos4210-origen.dtb \
 	exynos4210-trats.dtb \
 	exynos5250-smdk5250.dtb
 dtb-$(CONFIG_ARCH_HIGHBANK) += highbank.dtb
+dtb-$(CONFIG_ARCH_INTEGRATOR) += integratorap.dtb \
+	integratorcp.dtb
 dtb-$(CONFIG_ARCH_LPC32XX) += ea3250.dtb phy3250.dtb
 dtb-$(CONFIG_ARCH_KIRKWOOD) += kirkwood-dns320.dtb \
 	kirkwood-dns325.dtb \
-- 
1.7.11.4

^ permalink raw reply related

* [PATCH RFC 6/6 v3] gpio-generic: Add block GPIO API
From: Roland Stigge @ 2012-10-12 19:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1350069085-13283-1-git-send-email-stigge@antcom.de>

This patch adds block GPIO API support to the gpio-generic driver.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
 drivers/gpio/gpio-generic.c |   56 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

--- linux-2.6.orig/drivers/gpio/gpio-generic.c
+++ linux-2.6/drivers/gpio/gpio-generic.c
@@ -122,6 +122,13 @@ static int bgpio_get(struct gpio_chip *g
 	return bgc->read_reg(bgc->reg_dat) & bgc->pin2mask(bgc, gpio);
 }
 
+static unsigned bgpio_get_block(struct gpio_chip *gc, unsigned mask)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+
+	return bgc->read_reg(bgc->reg_dat) & mask;
+}
+
 static void bgpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
 {
 	struct bgpio_chip *bgc = to_bgpio_chip(gc);
@@ -170,6 +177,51 @@ static void bgpio_set_set(struct gpio_ch
 	spin_unlock_irqrestore(&bgc->lock, flags);
 }
 
+static void bgpio_set_block(struct gpio_chip *gc, unsigned mask,
+			    unsigned values)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	bgc->data &= ~mask;
+	bgc->data |= values & mask;
+
+	bgc->write_reg(bgc->reg_dat, bgc->data);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
+static void bgpio_set_with_clear_block(struct gpio_chip *gc, unsigned mask,
+				       unsigned values)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned set_bits = values & mask;
+	unsigned clr_bits = ~values & mask;
+
+	if (set_bits)
+		bgc->write_reg(bgc->reg_set, set_bits);
+	if (clr_bits)
+		bgc->write_reg(bgc->reg_set, clr_bits);
+}
+
+static void bgpio_set_set_block(struct gpio_chip *gc, unsigned mask,
+				unsigned values)
+{
+	struct bgpio_chip *bgc = to_bgpio_chip(gc);
+	unsigned long flags;
+
+	spin_lock_irqsave(&bgc->lock, flags);
+
+	bgc->data &= ~mask;
+	bgc->data |= values & mask;
+
+	bgc->write_reg(bgc->reg_set, bgc->data);
+
+	spin_unlock_irqrestore(&bgc->lock, flags);
+}
+
 static int bgpio_simple_dir_in(struct gpio_chip *gc, unsigned int gpio)
 {
 	return 0;
@@ -317,14 +369,18 @@ static int bgpio_setup_io(struct bgpio_c
 		bgc->reg_set = set;
 		bgc->reg_clr = clr;
 		bgc->gc.set = bgpio_set_with_clear;
+		bgc->gc.set_block = bgpio_set_with_clear_block;
 	} else if (set && !clr) {
 		bgc->reg_set = set;
 		bgc->gc.set = bgpio_set_set;
+		bgc->gc.set_block = bgpio_set_set_block;
 	} else {
 		bgc->gc.set = bgpio_set;
+		bgc->gc.set_block = bgpio_set_block;
 	}
 
 	bgc->gc.get = bgpio_get;
+	bgc->gc.get_block = bgpio_get_block;
 
 	return 0;
 }

^ permalink raw reply

* [PATCH RFC 5/6 v3] gpio-lpc32xx: Add block GPIO API
From: Roland Stigge @ 2012-10-12 19:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1350069085-13283-1-git-send-email-stigge@antcom.de>

This patch adds block GPIO API support to the LPC32xx driver.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
 drivers/gpio/gpio-lpc32xx.c |   78 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

--- linux-2.6.orig/drivers/gpio/gpio-lpc32xx.c
+++ linux-2.6/drivers/gpio/gpio-lpc32xx.c
@@ -297,6 +297,21 @@ static int lpc32xx_gpio_get_value_p3(str
 	return __get_gpio_state_p3(group, pin);
 }
 
+static unsigned lpc32xx_gpio_get_block_p3(struct gpio_chip *chip, unsigned mask)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 bits = __raw_readl(group->gpio_grp->inp_state);
+
+	/* In P3_INP_STATE, the 6 GPIOs are scattered over the register,
+	 * rearranging to bits 0-5
+	 */
+	bits >>= 10;
+	bits &= 0x401F;
+	bits |= (bits & 0x4000) >> 9;
+
+	return bits & mask & 0x3F;
+}
+
 static int lpc32xx_gpi_get_value(struct gpio_chip *chip, unsigned pin)
 {
 	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
@@ -304,6 +319,14 @@ static int lpc32xx_gpi_get_value(struct
 	return __get_gpi_state_p3(group, pin);
 }
 
+static unsigned lpc32xx_gpi_get_block(struct gpio_chip *chip, unsigned mask)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 bits = __raw_readl(group->gpio_grp->inp_state);
+
+	return bits & mask;
+}
+
 static int lpc32xx_gpio_dir_output_p012(struct gpio_chip *chip, unsigned pin,
 	int value)
 {
@@ -351,6 +374,26 @@ static void lpc32xx_gpio_set_value_p3(st
 	__set_gpio_level_p3(group, pin, value);
 }
 
+static void lpc32xx_gpio_set_block_p3(struct gpio_chip *chip, unsigned mask,
+				      unsigned values)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 set_bits = values & mask;
+	u32 clr_bits = ~values & mask;
+
+	/* States of GPIO 0-5 start at bit 25 */
+	set_bits <<= 25;
+	clr_bits <<= 25;
+
+	/* Note: On LPC32xx, GPOs can only be set at once or cleared at once,
+	 *       but not set and cleared@once
+	 */
+	if (set_bits)
+		__raw_writel(set_bits, group->gpio_grp->outp_set);
+	if (clr_bits)
+		__raw_writel(clr_bits, group->gpio_grp->outp_clr);
+}
+
 static void lpc32xx_gpo_set_value(struct gpio_chip *chip, unsigned pin,
 	int value)
 {
@@ -366,6 +409,30 @@ static int lpc32xx_gpo_get_value(struct
 	return __get_gpo_state_p3(group, pin);
 }
 
+static void lpc32xx_gpo_set_block(struct gpio_chip *chip, unsigned mask,
+				  unsigned values)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 set_bits = values & mask;
+	u32 clr_bits = ~values & mask;
+
+	/* Note: On LPC32xx, GPOs can only be set at once or cleared at once,
+	 *       but not set and cleared at once
+	 */
+	if (set_bits)
+		__raw_writel(set_bits, group->gpio_grp->outp_set);
+	if (clr_bits)
+		__raw_writel(clr_bits, group->gpio_grp->outp_clr);
+}
+
+static unsigned lpc32xx_gpo_get_block(struct gpio_chip *chip, unsigned mask)
+{
+	struct lpc32xx_gpio_chip *group = to_lpc32xx_gpio(chip);
+	u32 bits = __raw_readl(group->gpio_grp->outp_state);
+
+	return bits & mask;
+}
+
 static int lpc32xx_gpio_request(struct gpio_chip *chip, unsigned pin)
 {
 	if (pin < chip->ngpio)
@@ -440,8 +507,10 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpio_p0",
 			.direction_input	= lpc32xx_gpio_dir_input_p012,
 			.get			= lpc32xx_gpio_get_value_p012,
+			.get_block		= lpc32xx_gpi_get_block,
 			.direction_output	= lpc32xx_gpio_dir_output_p012,
 			.set			= lpc32xx_gpio_set_value_p012,
+			.set_block		= lpc32xx_gpo_set_block,
 			.request		= lpc32xx_gpio_request,
 			.to_irq			= lpc32xx_gpio_to_irq_p01,
 			.base			= LPC32XX_GPIO_P0_GRP,
@@ -456,8 +525,10 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpio_p1",
 			.direction_input	= lpc32xx_gpio_dir_input_p012,
 			.get			= lpc32xx_gpio_get_value_p012,
+			.get_block		= lpc32xx_gpi_get_block,
 			.direction_output	= lpc32xx_gpio_dir_output_p012,
 			.set			= lpc32xx_gpio_set_value_p012,
+			.set_block		= lpc32xx_gpo_set_block,
 			.request		= lpc32xx_gpio_request,
 			.to_irq			= lpc32xx_gpio_to_irq_p01,
 			.base			= LPC32XX_GPIO_P1_GRP,
@@ -472,8 +543,10 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpio_p2",
 			.direction_input	= lpc32xx_gpio_dir_input_p012,
 			.get			= lpc32xx_gpio_get_value_p012,
+			.get_block		= lpc32xx_gpi_get_block,
 			.direction_output	= lpc32xx_gpio_dir_output_p012,
 			.set			= lpc32xx_gpio_set_value_p012,
+			.set_block		= lpc32xx_gpo_set_block,
 			.request		= lpc32xx_gpio_request,
 			.base			= LPC32XX_GPIO_P2_GRP,
 			.ngpio			= LPC32XX_GPIO_P2_MAX,
@@ -487,8 +560,10 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpio_p3",
 			.direction_input	= lpc32xx_gpio_dir_input_p3,
 			.get			= lpc32xx_gpio_get_value_p3,
+			.get_block		= lpc32xx_gpio_get_block_p3,
 			.direction_output	= lpc32xx_gpio_dir_output_p3,
 			.set			= lpc32xx_gpio_set_value_p3,
+			.set_block		= lpc32xx_gpio_set_block_p3,
 			.request		= lpc32xx_gpio_request,
 			.to_irq			= lpc32xx_gpio_to_irq_gpio_p3,
 			.base			= LPC32XX_GPIO_P3_GRP,
@@ -503,6 +578,7 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpi_p3",
 			.direction_input	= lpc32xx_gpio_dir_in_always,
 			.get			= lpc32xx_gpi_get_value,
+			.get_block		= lpc32xx_gpi_get_block,
 			.request		= lpc32xx_gpio_request,
 			.to_irq			= lpc32xx_gpio_to_irq_gpi_p3,
 			.base			= LPC32XX_GPI_P3_GRP,
@@ -517,7 +593,9 @@ static struct lpc32xx_gpio_chip lpc32xx_
 			.label			= "gpo_p3",
 			.direction_output	= lpc32xx_gpio_dir_out_always,
 			.set			= lpc32xx_gpo_set_value,
+			.set_block		= lpc32xx_gpo_set_block,
 			.get			= lpc32xx_gpo_get_value,
+			.get_block		= lpc32xx_gpo_get_block,
 			.request		= lpc32xx_gpio_request,
 			.base			= LPC32XX_GPO_P3_GRP,
 			.ngpio			= LPC32XX_GPO_P3_MAX,

^ permalink raw reply

* [PATCH RFC 4/6 v3] gpio-max730x: Add block GPIO API
From: Roland Stigge @ 2012-10-12 19:11 UTC (permalink / raw)
  To: linux-arm-kernel
In-Reply-To: <1350069085-13283-1-git-send-email-stigge@antcom.de>

This patch adds block GPIO API support to the MAX730x driver.

Due to hardware constraints in this chip, simultaneous access to GPIO lines can
only be done in groups of 8: GPIOs 0-7, 8-15, 16-23, 24-27. However, setting
and clearing will be done at once.

Signed-off-by: Roland Stigge <stigge@antcom.de>

---
 drivers/gpio/gpio-max730x.c |   60 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

--- linux-2.6.orig/drivers/gpio/gpio-max730x.c
+++ linux-2.6/drivers/gpio/gpio-max730x.c
@@ -146,6 +146,43 @@ static int max7301_get(struct gpio_chip
 	return level;
 }
 
+static unsigned max7301_get_block(struct gpio_chip *chip, unsigned mask)
+{
+	struct max7301 *ts = container_of(chip, struct max7301, chip);
+	int i, j;
+	unsigned result = 0;
+
+	for (i = 0; i < 4; i++) {
+		if ((mask >> (i * 8)) & 0xFF) { /* i/o only if necessary */
+			u8 in_level = ts->read(ts->dev, 0x44 + i * 8);
+			u8 in_mask = 0;
+			u8 out_level = (ts->out_level >> (i * 8 + 4)) & 0xFF;
+			u8 out_mask = 0;
+
+			for (j = 0; j < 8; j++) {
+				int offset = 4 + i * 8 + j;
+				int config = (ts->port_config[offset >> 2] >>
+					      ((offset & 3) << 1)) &
+					PIN_CONFIG_MASK;
+
+				switch (config) {
+				case PIN_CONFIG_OUT:
+					out_mask |= BIT(j);
+					break;
+				case PIN_CONFIG_IN_WO_PULLUP:
+				case PIN_CONFIG_IN_PULLUP:
+					in_mask |= BIT(j);
+				}
+			}
+
+			result |= ((unsigned)(in_level & in_mask) |
+				   (out_level & out_mask)) << (i * 8);
+		}
+	}
+
+	return result & mask;
+}
+
 static void max7301_set(struct gpio_chip *chip, unsigned offset, int value)
 {
 	struct max7301 *ts = container_of(chip, struct max7301, chip);
@@ -160,6 +197,27 @@ static void max7301_set(struct gpio_chip
 	mutex_unlock(&ts->lock);
 }
 
+static
+void max7301_set_block(struct gpio_chip *chip, unsigned mask, unsigned values)
+{
+	struct max7301 *ts = container_of(chip, struct max7301, chip);
+	unsigned changes;
+	int i;
+
+	mutex_lock(&ts->lock);
+
+	changes = (ts->out_level ^ (values << 4)) & (mask << 4);
+	ts->out_level ^= changes;
+
+	for (i = 0; i < 4; i++) {
+		if ((changes >> (i * 8 + 4)) & 0xFF) /* i/o only on change */
+			ts->write(ts->dev, 0x44 + i * 8,
+				  (ts->out_level >> (i * 8 + 4)) & 0xFF);
+	}
+
+	mutex_unlock(&ts->lock);
+}
+
 int __devinit __max730x_probe(struct max7301 *ts)
 {
 	struct device *dev = ts->dev;
@@ -183,8 +241,10 @@ int __devinit __max730x_probe(struct max
 
 	ts->chip.direction_input = max7301_direction_input;
 	ts->chip.get = max7301_get;
+	ts->chip.get_block = max7301_get_block;
 	ts->chip.direction_output = max7301_direction_output;
 	ts->chip.set = max7301_set;
+	ts->chip.set_block = max7301_set_block;
 
 	ts->chip.base = pdata->base;
 	ts->chip.ngpio = PIN_NUMBER;

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox