Re: [PATCH] ARM: SAMSUNG: devs: Add names to fimd0 IRQ resources
Thursday, May 16, 2013 12:10 AM, Tomasz Figa wrote: Since commit 1977e6d8 (drm/exynos: change the method for getting the interrupt) the Exynos DRM FIMD driver requires IRQ resources to be named. This patch fixes probe failure in non-DT cases by adding appropriate resource names to fimd0 platform device. Signed-off-by: Tomasz Figa t.f...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com It looks good. Thank you for sending the patch. Acked-by: Jingoo Han jg1@samsung.com Best regards, Jingoo Han --- arch/arm/plat-samsung/devs.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/arch/arm/plat-samsung/devs.c b/arch/arm/plat-samsung/devs.c index 30c2fe2..0f9c3f4 100644 --- a/arch/arm/plat-samsung/devs.c +++ b/arch/arm/plat-samsung/devs.c @@ -311,9 +311,9 @@ struct platform_device s5p_device_jpeg = { #ifdef CONFIG_S5P_DEV_FIMD0 static struct resource s5p_fimd0_resource[] = { [0] = DEFINE_RES_MEM(S5P_PA_FIMD0, SZ_32K), - [1] = DEFINE_RES_IRQ(IRQ_FIMD0_VSYNC), - [2] = DEFINE_RES_IRQ(IRQ_FIMD0_FIFO), - [3] = DEFINE_RES_IRQ(IRQ_FIMD0_SYSTEM), + [1] = DEFINE_RES_IRQ_NAMED(IRQ_FIMD0_VSYNC, vsync), + [2] = DEFINE_RES_IRQ_NAMED(IRQ_FIMD0_FIFO, fifo), + [3] = DEFINE_RES_IRQ_NAMED(IRQ_FIMD0_SYSTEM, lcd_sys), }; struct platform_device s5p_device_fimd0 = { -- 1.8.2.1
[PATCH v2 0/6] Samsung USB PHY SoC support cleanup
This patch series intends to improve handling of SoC-specific differences in Samsung USB PHY drivers, by reducing the need to explicitly check SoC type using if and switch statements. In addition, last patch adds support for Exynos 4x12, as this is simply a matter of adding appropriate driver data and additional case in two switches. Tested on Exynos4210-based Trats board and Exynos4412-based internal Samsung board. Changes since v1: - rebased to latest usb-next Tomasz Figa (6): usb: phy: samsung: Select common driver part implicitly usb: phy: samsung: Use clk_get to get reference clock usb: phy: samsung: Consolidate reference clock rate handling usb: phy: samsung: Pass set_isolation callback through driver data usb: phy: samsung: Pass enable/disable callbacks through driver data usb: phy: samsung: Add support for USB 2.0 PHY on Exynos 4x12 drivers/usb/phy/Kconfig| 2 +- drivers/usb/phy/phy-samsung-usb.c | 154 ++--- drivers/usb/phy/phy-samsung-usb.h | 14 +++- drivers/usb/phy/phy-samsung-usb2.c | 53 + drivers/usb/phy/phy-samsung-usb3.c | 23 -- 5 files changed, 146 insertions(+), 100 deletions(-) -- 1.8.2.1 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/6] usb: phy: samsung: Select common driver part implicitly
Since phy-samsung-usb library can be used only by phy-samsung-usb2 and phy-samsung-usb3 drivers, there is no need to give explicit control over its Kconfig symbol. This patch makes CONFIG_SAMSUNG_USBPHY symbol hidden and selected implicitly by CONFIG_SAMSUNG_USB2PHY and CONFIG_SAMSUNG_USB3PHY. Signed-off-by: Tomasz Figa t.f...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/usb/phy/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig index 371d0e7..a2b9e18 100644 --- a/drivers/usb/phy/Kconfig +++ b/drivers/usb/phy/Kconfig @@ -86,7 +86,7 @@ config OMAP_USB3 on/off the PHY. config SAMSUNG_USBPHY - tristate Samsung USB PHY Driver + tristate help Enable this to support Samsung USB phy helper driver for Samsung SoCs. This driver provides common interface to interact, for Samsung USB 2.0 PHY -- 1.8.2.1 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 2/6] usb: phy: samsung: Use clk_get to get reference clock
There is no need to use devm_clk_get to get a clock that is being put at the end of the function. This patch changes the code getting reference clock to use clk_get instead of useless in this case devm_clk_get. Signed-off-by: Tomasz Figa t.f...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/usb/phy/phy-samsung-usb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/phy/phy-samsung-usb.c b/drivers/usb/phy/phy-samsung-usb.c index 7b118ee5..62cdb7e 100644 --- a/drivers/usb/phy/phy-samsung-usb.c +++ b/drivers/usb/phy/phy-samsung-usb.c @@ -175,9 +175,9 @@ int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy) * external crystal clock XXTI */ if (sphy-drv_data-cpu_type == TYPE_EXYNOS5250) - ref_clk = devm_clk_get(sphy-dev, ext_xtal); + ref_clk = clk_get(sphy-dev, ext_xtal); else - ref_clk = devm_clk_get(sphy-dev, xusbxti); + ref_clk = clk_get(sphy-dev, xusbxti); if (IS_ERR(ref_clk)) { dev_err(sphy-dev, Failed to get reference clock\n); return PTR_ERR(ref_clk); -- 1.8.2.1 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/6] usb: phy: samsung: Pass set_isolation callback through driver data
This patch extends driver data structure with set_isolation callback, which allows to remove the need for checking for SoC type in a switch statement. Signed-off-by: Tomasz Figa t.f...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/usb/phy/phy-samsung-usb.c | 36 drivers/usb/phy/phy-samsung-usb.h | 4 +++- drivers/usb/phy/phy-samsung-usb2.c | 11 +++ drivers/usb/phy/phy-samsung-usb3.c | 7 +-- 4 files changed, 23 insertions(+), 35 deletions(-) diff --git a/drivers/usb/phy/phy-samsung-usb.c b/drivers/usb/phy/phy-samsung-usb.c index c40ea32..7a1ed90 100644 --- a/drivers/usb/phy/phy-samsung-usb.c +++ b/drivers/usb/phy/phy-samsung-usb.c @@ -73,7 +73,7 @@ EXPORT_SYMBOL_GPL(samsung_usbphy_parse_dt); * Here 'on = true' would mean USB PHY block is isolated, hence * de-activated and vice-versa. */ -void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, bool on) +void samsung_usbphy_set_isolation_4210(struct samsung_usbphy *sphy, bool on) { void __iomem *reg = NULL; u32 reg_val; @@ -84,32 +84,12 @@ void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, bool on) return; } - switch (sphy-drv_data-cpu_type) { - case TYPE_S3C64XX: - /* -* Do nothing: We will add here once S3C64xx goes for DT support -*/ - break; - case TYPE_EXYNOS4210: - /* -* Fall through since exynos4210 and exynos5250 have similar -* register architecture: two separate registers for host and -* device phy control with enable bit at position 0. -*/ - case TYPE_EXYNOS5250: - if (sphy-phy_type == USB_PHY_TYPE_DEVICE) { - reg = sphy-pmuregs + - sphy-drv_data-devphy_reg_offset; - en_mask = sphy-drv_data-devphy_en_mask; - } else if (sphy-phy_type == USB_PHY_TYPE_HOST) { - reg = sphy-pmuregs + - sphy-drv_data-hostphy_reg_offset; - en_mask = sphy-drv_data-hostphy_en_mask; - } - break; - default: - dev_err(sphy-dev, Invalid SoC type\n); - return; + if (sphy-phy_type == USB_PHY_TYPE_DEVICE) { + reg = sphy-pmuregs + sphy-drv_data-devphy_reg_offset; + en_mask = sphy-drv_data-devphy_en_mask; + } else if (sphy-phy_type == USB_PHY_TYPE_HOST) { + reg = sphy-pmuregs + sphy-drv_data-hostphy_reg_offset; + en_mask = sphy-drv_data-hostphy_en_mask; } reg_val = readl(reg); @@ -121,7 +101,7 @@ void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, bool on) writel(reg_val, reg); } -EXPORT_SYMBOL_GPL(samsung_usbphy_set_isolation); +EXPORT_SYMBOL_GPL(samsung_usbphy_set_isolation_4210); /* * Configure the mode of working of usb-phy here: HOST/DEVICE. diff --git a/drivers/usb/phy/phy-samsung-usb.h b/drivers/usb/phy/phy-samsung-usb.h index 0336f6b..5203784 100644 --- a/drivers/usb/phy/phy-samsung-usb.h +++ b/drivers/usb/phy/phy-samsung-usb.h @@ -271,6 +271,7 @@ struct samsung_usbphy_drvdata { u32 devphy_reg_offset; u32 hostphy_reg_offset; int (*rate_to_clksel)(struct samsung_usbphy *, unsigned long); + void (*set_isolation)(struct samsung_usbphy *, bool); }; /* @@ -323,7 +324,8 @@ static inline const struct samsung_usbphy_drvdata } extern int samsung_usbphy_parse_dt(struct samsung_usbphy *sphy); -extern void samsung_usbphy_set_isolation(struct samsung_usbphy *sphy, bool on); +extern void samsung_usbphy_set_isolation_4210(struct samsung_usbphy *sphy, + bool on); extern void samsung_usbphy_cfg_sel(struct samsung_usbphy *sphy); extern int samsung_usbphy_set_type(struct usb_phy *phy, enum samsung_usb_phy_type phy_type); diff --git a/drivers/usb/phy/phy-samsung-usb2.c b/drivers/usb/phy/phy-samsung-usb2.c index 802e738..ae6da68 100644 --- a/drivers/usb/phy/phy-samsung-usb2.c +++ b/drivers/usb/phy/phy-samsung-usb2.c @@ -284,8 +284,8 @@ static int samsung_usb2phy_init(struct usb_phy *phy) /* Disable phy isolation */ if (sphy-plat sphy-plat-pmu_isolation) sphy-plat-pmu_isolation(false); - else - samsung_usbphy_set_isolation(sphy, false); + else if (sphy-drv_data-set_isolation) + sphy-drv_data-set_isolation(sphy, false); /* Selecting Host/OTG mode; After reset USB2.0PHY_CFG: HOST */ samsung_usbphy_cfg_sel(sphy); @@ -342,8 +342,8 @@ static void samsung_usb2phy_shutdown(struct usb_phy *phy) /* Enable phy isolation */ if (sphy-plat sphy-plat-pmu_isolation)
[PATCH v2 3/6] usb: phy: samsung: Consolidate reference clock rate handling
This patch cleans up handling of reference clock rate in Samsung USB PHY drivers. It is mostly a cosmetic change but improves error handling in case of failing to get reference clock or invalid clock rate. Signed-off-by: Tomasz Figa t.f...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/usb/phy/phy-samsung-usb.c | 114 ++--- drivers/usb/phy/phy-samsung-usb.h | 7 +++ drivers/usb/phy/phy-samsung-usb2.c | 8 ++- drivers/usb/phy/phy-samsung-usb3.c | 6 +- 4 files changed, 86 insertions(+), 49 deletions(-) diff --git a/drivers/usb/phy/phy-samsung-usb.c b/drivers/usb/phy/phy-samsung-usb.c index 62cdb7e..c40ea32 100644 --- a/drivers/usb/phy/phy-samsung-usb.c +++ b/drivers/usb/phy/phy-samsung-usb.c @@ -162,13 +162,76 @@ int samsung_usbphy_set_type(struct usb_phy *phy, } EXPORT_SYMBOL_GPL(samsung_usbphy_set_type); +int samsung_usbphy_rate_to_clksel_64xx(struct samsung_usbphy *sphy, + unsigned long rate) +{ + unsigned int clksel; + + switch (rate) { + case 12 * MHZ: + clksel = PHYCLK_CLKSEL_12M; + break; + case 24 * MHZ: + clksel = PHYCLK_CLKSEL_24M; + break; + case 48 * MHZ: + clksel = PHYCLK_CLKSEL_48M; + break; + default: + dev_err(sphy-dev, + Invalid reference clock frequency: %lu\n, rate); + return -EINVAL; + } + + return clksel; +} +EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_64xx); + +int samsung_usbphy_rate_to_clksel_4x12(struct samsung_usbphy *sphy, + unsigned long rate) +{ + unsigned int clksel; + + switch (rate) { + case 9600 * KHZ: + clksel = FSEL_CLKSEL_9600K; + break; + case 10 * MHZ: + clksel = FSEL_CLKSEL_10M; + break; + case 12 * MHZ: + clksel = FSEL_CLKSEL_12M; + break; + case 19200 * KHZ: + clksel = FSEL_CLKSEL_19200K; + break; + case 20 * MHZ: + clksel = FSEL_CLKSEL_20M; + break; + case 24 * MHZ: + clksel = FSEL_CLKSEL_24M; + break; + case 50 * MHZ: + clksel = FSEL_CLKSEL_50M; + break; + default: + dev_err(sphy-dev, + Invalid reference clock frequency: %lu\n, rate); + return -EINVAL; + } + + return clksel; +} +EXPORT_SYMBOL_GPL(samsung_usbphy_rate_to_clksel_4x12); + /* * Returns reference clock frequency selection value */ int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy) { struct clk *ref_clk; - int refclk_freq = 0; + unsigned long rate; + int refclk_freq; /* * In exynos5250 USB host and device PHY use @@ -183,52 +246,9 @@ int samsung_usbphy_get_refclk_freq(struct samsung_usbphy *sphy) return PTR_ERR(ref_clk); } - if (sphy-drv_data-cpu_type == TYPE_EXYNOS5250) { - /* set clock frequency for PLL */ - switch (clk_get_rate(ref_clk)) { - case 9600 * KHZ: - refclk_freq = FSEL_CLKSEL_9600K; - break; - case 10 * MHZ: - refclk_freq = FSEL_CLKSEL_10M; - break; - case 12 * MHZ: - refclk_freq = FSEL_CLKSEL_12M; - break; - case 19200 * KHZ: - refclk_freq = FSEL_CLKSEL_19200K; - break; - case 20 * MHZ: - refclk_freq = FSEL_CLKSEL_20M; - break; - case 50 * MHZ: - refclk_freq = FSEL_CLKSEL_50M; - break; - case 24 * MHZ: - default: - /* default reference clock */ - refclk_freq = FSEL_CLKSEL_24M; - break; - } - } else { - switch (clk_get_rate(ref_clk)) { - case 12 * MHZ: - refclk_freq = PHYCLK_CLKSEL_12M; - break; - case 24 * MHZ: - refclk_freq = PHYCLK_CLKSEL_24M; - break; - case 48 * MHZ: - refclk_freq = PHYCLK_CLKSEL_48M; - break; - default: - if (sphy-drv_data-cpu_type == TYPE_S3C64XX) - refclk_freq = PHYCLK_CLKSEL_48M; - else - refclk_freq = PHYCLK_CLKSEL_24M; - break; - } - } + rate =
[PATCH v2 5/6] usb: phy: samsung: Pass enable/disable callbacks through driver data
To remove unnecessary if statements, this patch introduces phy_enable and phy_disable callbacks in driver data structure that implement SoC-specific PHY initialization and deinitialization. Signed-off-by: Tomasz Figa t.f...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/usb/phy/phy-samsung-usb.h | 2 ++ drivers/usb/phy/phy-samsung-usb2.c | 16 drivers/usb/phy/phy-samsung-usb3.c | 10 +- 3 files changed, 15 insertions(+), 13 deletions(-) diff --git a/drivers/usb/phy/phy-samsung-usb.h b/drivers/usb/phy/phy-samsung-usb.h index 5203784..31e2ec3 100644 --- a/drivers/usb/phy/phy-samsung-usb.h +++ b/drivers/usb/phy/phy-samsung-usb.h @@ -272,6 +272,8 @@ struct samsung_usbphy_drvdata { u32 hostphy_reg_offset; int (*rate_to_clksel)(struct samsung_usbphy *, unsigned long); void (*set_isolation)(struct samsung_usbphy *, bool); + void (*phy_enable)(struct samsung_usbphy *); + void (*phy_disable)(struct samsung_usbphy *); }; /* diff --git a/drivers/usb/phy/phy-samsung-usb2.c b/drivers/usb/phy/phy-samsung-usb2.c index ae6da68..b81347b 100644 --- a/drivers/usb/phy/phy-samsung-usb2.c +++ b/drivers/usb/phy/phy-samsung-usb2.c @@ -291,10 +291,7 @@ static int samsung_usb2phy_init(struct usb_phy *phy) samsung_usbphy_cfg_sel(sphy); /* Initialize usb phy registers */ - if (sphy-drv_data-cpu_type == TYPE_EXYNOS5250) - samsung_exynos5_usb2phy_enable(sphy); - else - samsung_usb2phy_enable(sphy); + sphy-drv_data-phy_enable(sphy); spin_unlock_irqrestore(sphy-lock, flags); @@ -334,10 +331,7 @@ static void samsung_usb2phy_shutdown(struct usb_phy *phy) } /* De-initialize usb phy registers */ - if (sphy-drv_data-cpu_type == TYPE_EXYNOS5250) - samsung_exynos5_usb2phy_disable(sphy); - else - samsung_usb2phy_disable(sphy); + sphy-drv_data-phy_disable(sphy); /* Enable phy isolation */ if (sphy-plat sphy-plat-pmu_isolation) @@ -448,6 +442,8 @@ static const struct samsung_usbphy_drvdata usb2phy_s3c64xx = { .devphy_en_mask = S3C64XX_USBPHY_ENABLE, .rate_to_clksel = samsung_usbphy_rate_to_clksel_64xx, .set_isolation = NULL, /* TODO */ + .phy_enable = samsung_usb2phy_enable, + .phy_disable= samsung_usb2phy_disable, }; static const struct samsung_usbphy_drvdata usb2phy_exynos4 = { @@ -456,6 +452,8 @@ static const struct samsung_usbphy_drvdata usb2phy_exynos4 = { .hostphy_en_mask= EXYNOS_USBPHY_ENABLE, .rate_to_clksel = samsung_usbphy_rate_to_clksel_64xx, .set_isolation = samsung_usbphy_set_isolation_4210, + .phy_enable = samsung_usb2phy_enable, + .phy_disable= samsung_usb2phy_disable, }; static struct samsung_usbphy_drvdata usb2phy_exynos5 = { @@ -464,6 +462,8 @@ static struct samsung_usbphy_drvdata usb2phy_exynos5 = { .hostphy_reg_offset = EXYNOS_USBHOST_PHY_CTRL_OFFSET, .rate_to_clksel = samsung_usbphy_rate_to_clksel_4x12, .set_isolation = samsung_usbphy_set_isolation_4210, + .phy_enable = samsung_exynos5_usb2phy_enable, + .phy_disable= samsung_exynos5_usb2phy_disable, }; #ifdef CONFIG_OF diff --git a/drivers/usb/phy/phy-samsung-usb3.c b/drivers/usb/phy/phy-samsung-usb3.c index ae232ad..b663353 100644 --- a/drivers/usb/phy/phy-samsung-usb3.c +++ b/drivers/usb/phy/phy-samsung-usb3.c @@ -65,7 +65,7 @@ static u32 samsung_usb3phy_set_refclk(struct samsung_usbphy *sphy) return reg; } -static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy) +static void samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy) { void __iomem *regs = sphy-regs; u32 phyparam0; @@ -133,8 +133,6 @@ static int samsung_exynos5_usb3phy_enable(struct samsung_usbphy *sphy) phyclkrst = ~(PHYCLKRST_PORTRESET); writel(phyclkrst, regs + EXYNOS5_DRD_PHYCLKRST); - - return 0; } static void samsung_exynos5_usb3phy_disable(struct samsung_usbphy *sphy) @@ -188,7 +186,7 @@ static int samsung_usb3phy_init(struct usb_phy *phy) sphy-drv_data-set_isolation(sphy, false); /* Initialize usb phy registers */ - samsung_exynos5_usb3phy_enable(sphy); + sphy-drv_data-phy_enable(sphy); spin_unlock_irqrestore(sphy-lock, flags); @@ -219,7 +217,7 @@ static void samsung_usb3phy_shutdown(struct usb_phy *phy) samsung_usbphy_set_type(sphy-phy, USB_PHY_TYPE_DEVICE); /* De-initialize usb phy registers */ - samsung_exynos5_usb3phy_disable(sphy); + sphy-drv_data-phy_disable(sphy); /* Enable phy isolation */ if (sphy-drv_data-set_isolation) @@ -312,6 +310,8 @@ static struct samsung_usbphy_drvdata usb3phy_exynos5 = {
[PATCH v2 6/6] usb: phy: samsung: Add support for USB 2.0 PHY on Exynos 4x12
This patch adds driver data for Exynos 4x12 USB 2.0 PHY. Signed-off-by: Tomasz Figa t.f...@samsung.com Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com --- drivers/usb/phy/phy-samsung-usb.h | 1 + drivers/usb/phy/phy-samsung-usb2.c | 18 ++ 2 files changed, 19 insertions(+) diff --git a/drivers/usb/phy/phy-samsung-usb.h b/drivers/usb/phy/phy-samsung-usb.h index 31e2ec3..585d12f 100644 --- a/drivers/usb/phy/phy-samsung-usb.h +++ b/drivers/usb/phy/phy-samsung-usb.h @@ -241,6 +241,7 @@ enum samsung_cpu_type { TYPE_S3C64XX, TYPE_EXYNOS4210, + TYPE_EXYNOS4X12, TYPE_EXYNOS5250, }; diff --git a/drivers/usb/phy/phy-samsung-usb2.c b/drivers/usb/phy/phy-samsung-usb2.c index b81347b..381f8d4 100644 --- a/drivers/usb/phy/phy-samsung-usb2.c +++ b/drivers/usb/phy/phy-samsung-usb2.c @@ -177,6 +177,7 @@ static void samsung_usb2phy_enable(struct samsung_usbphy *sphy) rstcon |= RSTCON_SWRST; break; case TYPE_EXYNOS4210: + case TYPE_EXYNOS4X12: phypwr = ~PHYPWR_NORMAL_MASK_PHY0; rstcon |= RSTCON_SWRST; default: @@ -240,6 +241,7 @@ static void samsung_usb2phy_disable(struct samsung_usbphy *sphy) phypwr |= PHYPWR_NORMAL_MASK; break; case TYPE_EXYNOS4210: + case TYPE_EXYNOS4X12: phypwr |= PHYPWR_NORMAL_MASK_PHY0; default: break; @@ -456,6 +458,16 @@ static const struct samsung_usbphy_drvdata usb2phy_exynos4 = { .phy_disable= samsung_usb2phy_disable, }; +static const struct samsung_usbphy_drvdata usb2phy_exynos4x12 = { + .cpu_type = TYPE_EXYNOS4X12, + .devphy_en_mask = EXYNOS_USBPHY_ENABLE, + .hostphy_en_mask= EXYNOS_USBPHY_ENABLE, + .rate_to_clksel = samsung_usbphy_rate_to_clksel_4x12, + .set_isolation = samsung_usbphy_set_isolation_4210, + .phy_enable = samsung_usb2phy_enable, + .phy_disable= samsung_usb2phy_disable, +}; + static struct samsung_usbphy_drvdata usb2phy_exynos5 = { .cpu_type = TYPE_EXYNOS5250, .hostphy_en_mask= EXYNOS_USBPHY_ENABLE, @@ -475,6 +487,9 @@ static const struct of_device_id samsung_usbphy_dt_match[] = { .compatible = samsung,exynos4210-usb2phy, .data = usb2phy_exynos4, }, { + .compatible = samsung,exynos4x12-usb2phy, + .data = usb2phy_exynos4x12, + }, { .compatible = samsung,exynos5250-usb2phy, .data = usb2phy_exynos5 }, @@ -491,6 +506,9 @@ static struct platform_device_id samsung_usbphy_driver_ids[] = { .name = exynos4210-usb2phy, .driver_data= (unsigned long)usb2phy_exynos4, }, { + .name = exynos4x12-usb2phy, + .driver_data= (unsigned long)usb2phy_exynos4x12, + }, { .name = exynos5250-usb2phy, .driver_data= (unsigned long)usb2phy_exynos5, }, -- 1.8.2.1 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 30/33] arch/arm/plat-samsung: don't check resource with devm_ioremap_resource
devm_ioremap_resource does sanity checks on the given resource. No need to duplicate this in the driver. Signed-off-by: Wolfram Sang w...@the-dreams.de --- arch/arm/plat-samsung/adc.c |5 - 1 file changed, 5 deletions(-) diff --git a/arch/arm/plat-samsung/adc.c b/arch/arm/plat-samsung/adc.c index ca07cb1..79690f2 100644 --- a/arch/arm/plat-samsung/adc.c +++ b/arch/arm/plat-samsung/adc.c @@ -381,11 +381,6 @@ static int s3c_adc_probe(struct platform_device *pdev) } regs = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!regs) { - dev_err(dev, failed to find registers\n); - return -ENXIO; - } - adc-regs = devm_ioremap_resource(dev, regs); if (IS_ERR(adc-regs)) return PTR_ERR(adc-regs); -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 08/33] drivers/i2c/busses: don't check resource with devm_ioremap_resource
devm_ioremap_resource does sanity checks on the given resource. No need to duplicate this in the driver. Signed-off-by: Wolfram Sang w...@the-dreams.de --- drivers/i2c/busses/i2c-s3c2410.c |5 - drivers/i2c/busses/i2c-sirf.c|6 -- drivers/i2c/busses/i2c-tegra.c |5 - 3 files changed, 16 deletions(-) diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c index 6e8ee92..cab1c91 100644 --- a/drivers/i2c/busses/i2c-s3c2410.c +++ b/drivers/i2c/busses/i2c-s3c2410.c @@ -1082,11 +1082,6 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev) /* map the registers */ res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (res == NULL) { - dev_err(pdev-dev, cannot find IO resource\n); - return -ENOENT; - } - i2c-regs = devm_ioremap_resource(pdev-dev, res); if (IS_ERR(i2c-regs)) diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c index 5a7ad24..a63c7d5 100644 --- a/drivers/i2c/busses/i2c-sirf.c +++ b/drivers/i2c/busses/i2c-sirf.c @@ -303,12 +303,6 @@ static int i2c_sirfsoc_probe(struct platform_device *pdev) adap-class = I2C_CLASS_HWMON; mem_res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (mem_res == NULL) { - dev_err(pdev-dev, Unable to get MEM resource\n); - err = -EINVAL; - goto out; - } - siic-base = devm_ioremap_resource(pdev-dev, mem_res); if (IS_ERR(siic-base)) { err = PTR_ERR(siic-base); diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index b60ff90..9aa1b60 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++ b/drivers/i2c/busses/i2c-tegra.c @@ -714,11 +714,6 @@ static int tegra_i2c_probe(struct platform_device *pdev) int ret = 0; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - if (!res) { - dev_err(pdev-dev, no mem resource\n); - return -EINVAL; - } - base = devm_ioremap_resource(pdev-dev, res); if (IS_ERR(base)) return PTR_ERR(base); -- 1.7.10.4 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 08/33] drivers/i2c/busses: don't check resource with devm_ioremap_resource
2013/5/16 Wolfram Sang w...@the-dreams.de: devm_ioremap_resource does sanity checks on the given resource. No need to duplicate this in the driver. Signed-off-by: Wolfram Sang w...@the-dreams.de Acked-by: Barry Song baohua.s...@csr.com -barry -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] Fix suspend/resume issues created by pinmux on exynos
This set of patches fixes some problems with suspend/resume that were introduced by the switch from the old gpio code to the new pinmux code. Specifically: * It adds saving and restoring of pincontrol registers. * It fixes eint wakeups. This set of two patches was verified on a backport of the current pinmux code onto 3.8 on a Samsung ARM Chromebook. Suspend/resume does not seem functional on the ARM Chromebook on current ToT Linux so I couldn't validate there. This gets us one step closer, though! Since patches applied cleanly I'm fairly certain that they will work on ToT as well as they do in our tree. These patches have only been tested on exynos5250. I have made an effort to support other samsung boards (even those with two CONF registers), but that support is untested. Tomasz Figa has said that he has similar patches in development. I'm posting what we have here but if Tomasz's patches end up being more suitable I have no objections to taking them over these (or of Tomasz wants to merge the two?). If you'd like to see the gerrit reviews of these in the Chrome OS tree, you can see: * https://gerrit.chromium.org/gerrit/#/c/51336/4 * https://gerrit.chromium.org/gerrit/#/c/51342/3 Doug Anderson (1): pinctrl: samsung: fix suspend/resume functionality Prathyush K (1): pinctrl: exynos: fix eint wakeup by using irq_set_wake() drivers/pinctrl/pinctrl-exynos.c | 45 ++--- drivers/pinctrl/pinctrl-exynos.h | 3 +- drivers/pinctrl/pinctrl-samsung.c | 199 ++ drivers/pinctrl/pinctrl-samsung.h | 13 +++ 4 files changed, 247 insertions(+), 13 deletions(-) -- 1.8.2.1 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
The GPIO states need to be restored after s2r and this is not currently supported in the pinctrl driver. This patch saves the gpio states before suspend and restores them after resume. Logic and commenting for samsung_pinctrl_resume_noirq() is heavily borrowed from the old samsung_gpio_pm_2bit_resume(), which seemed to do this a reasonable way. Patch originally from Prathyush K prathyus...@samsung.com but rewritten by Doug Anderson diand...@chromium.org. Signed-off-by: Prathyush K prathyus...@samsung.com Signed-off-by: Doug Anderson diand...@chromium.org --- drivers/pinctrl/pinctrl-samsung.c | 199 ++ drivers/pinctrl/pinctrl-samsung.h | 11 +++ 2 files changed, 210 insertions(+) diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c index 9763668..0891667 100644 --- a/drivers/pinctrl/pinctrl-samsung.c +++ b/drivers/pinctrl/pinctrl-samsung.c @@ -964,6 +964,204 @@ static int samsung_pinctrl_probe(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM + +/** + * samsung_pinctrl_resume_noirq - save pinctrl state for suspend + * + * Save data for all banks handled by this device. + */ +static int samsung_pinctrl_suspend_noirq(struct device *dev) +{ + struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev); + struct samsung_pin_ctrl *ctrl = drvdata-ctrl; + void __iomem * const virt_base = drvdata-virt_base; + int i; + + for (i = 0; i ctrl-nr_banks; i++) { + struct samsung_pin_bank *bank = ctrl-pin_banks[i]; + const struct samsung_pin_bank_type *type = bank-type; + void __iomem * const reg = virt_base + bank-pctl_offset; + + bank-pm_save.con = readl(reg + + type-reg_offset[PINCFG_TYPE_FUNC]); + if (type-fld_width[PINCFG_TYPE_FUNC] 4) + bank-pm_save.con |= (u64)readl(reg + 4 + + type-reg_offset[PINCFG_TYPE_FUNC]) 32; + bank-pm_save.dat = readl(reg + + type-reg_offset[PINCFG_TYPE_DAT]); + bank-pm_save.pud = readl(reg + + type-reg_offset[PINCFG_TYPE_PUD]); + bank-pm_save.drv = readl(reg + + type-reg_offset[PINCFG_TYPE_DRV]); + + if (type-fld_width[PINCFG_TYPE_CON_PDN]) { + bank-pm_save.conpdn = readl(reg + + type-reg_offset[PINCFG_TYPE_CON_PDN]); + bank-pm_save.pudpdn = readl(reg + + type-reg_offset[PINCFG_TYPE_PUD_PDN]); + } + + dev_dbg(dev, Save %s @ %p (con %#010llx)\n, + bank-name, reg, bank-pm_save.con); + } + + return 0; +} + +/** + * is_sfn - test whether a pin config represents special function. + * + * Test whether the given masked+shifted bits of an GPIO configuration + * are one of the SFN (special function) modes. + */ +static inline int is_sfn(u32 con) +{ + return con = 2; +} + +/** + * is_in - test if the given masked+shifted GPIO configuration is an input. + */ +static inline int is_in(u32 con) +{ + return con == 0; +} + +/** + * is_out - test if the given masked+shifted GPIO configuration is an output. + */ +static inline int is_out(u32 con) +{ + return con == 1; +} + +/** + * samsung_pinctrl_resume_noirq - restore pinctrl state from suspend + * + * Restore one of the GPIO banks that was saved during suspend. This is + * not as simple as once thought, due to the possibility of glitches + * from the order that the CON and DAT registers are set in. + * + * The three states the pin can be are {IN,OUT,SFN} which gives us 9 + * combinations of changes to check. Three of these, if the pin stays + * in the same configuration can be discounted. This leaves us with + * the following: + * + * { IN = OUT } Change DAT first + * { IN = SFN } Change CON first + * { OUT = SFN } Change CON first, so new data will not glitch + * { OUT = IN } Change CON first, so new data will not glitch + * { SFN = IN } Change CON first + * { SFN = OUT } Change DAT first, so new data will not glitch [1] + * + * We do not currently deal with the UP registers as these control + * weak resistors, so a small delay in change should not need to bring + * these into the calculations. + * + * [1] this assumes that writing to a pin DAT whilst in SFN will set the + * state for when it is next output. + */ +static int samsung_pinctrl_resume_noirq(struct device *dev) +{ + struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev); + struct samsung_pin_ctrl *ctrl = drvdata-ctrl; + void __iomem * const virt_base = drvdata-virt_base; + int i; + + for (i = 0; i ctrl-nr_banks; i++) { + const struct samsung_pin_bank *bank = ctrl-pin_banks[i]; +
[PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()
From: Prathyush K prathyus...@samsung.com Add the irq_set_wake function for exynos pinctrl to configure the external interrupt wakeup mask register. [dianders: minor nit fixes; port to ToT] Signed-off-by: Prathyush K prathyus...@samsung.com Signed-off-by: Doug Anderson diand...@chromium.org --- drivers/pinctrl/pinctrl-exynos.c | 45 --- drivers/pinctrl/pinctrl-exynos.h | 3 ++- drivers/pinctrl/pinctrl-samsung.h | 2 ++ 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c index ac74281..3ebb2ff 100644 --- a/drivers/pinctrl/pinctrl-exynos.c +++ b/drivers/pinctrl/pinctrl-exynos.c @@ -30,6 +30,8 @@ #include linux/spinlock.h #include linux/err.h +#include plat/pm.h + #include pinctrl-samsung.h #include pinctrl-exynos.h @@ -326,6 +328,24 @@ static int exynos_wkup_irq_set_type(struct irq_data *irqd, unsigned int type) return 0; } +static int exynos_wkup_irq_set_wake(struct irq_data *irqd, unsigned int state) +{ + struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); + const int eint_num = bank-eint_base + irqd-hwirq; + unsigned long bit = 1L eint_num; + + pr_info(wake %s for eint %d / %s[%ld]\n, + state ? enabled : disabled, + eint_num, bank-name, irqd-hwirq); + + if (!state) + s3c_irqwake_eintmask |= bit; + else + s3c_irqwake_eintmask = ~bit; + + return 0; +} + /* * irq_chip for wakeup interrupts */ @@ -335,6 +355,7 @@ static struct irq_chip exynos_wkup_irq_chip = { .irq_mask = exynos_wkup_irq_mask, .irq_ack= exynos_wkup_irq_ack, .irq_set_type = exynos_wkup_irq_set_type, + .irq_set_wake = exynos_wkup_irq_set_wake, }; /* interrupt handler for wakeup interrupts 0..15 */ @@ -543,10 +564,10 @@ static struct samsung_pin_bank exynos4210_pin_banks1[] = { EXYNOS_PIN_BANK_EINTN(8, 0x1A0, gpy4), EXYNOS_PIN_BANK_EINTN(8, 0x1C0, gpy5), EXYNOS_PIN_BANK_EINTN(8, 0x1E0, gpy6), - EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00), - EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04), - EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08), - EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c), + EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00, 0), + EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04, 8), + EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08, 16), + EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c, 24), }; /* pin banks of exynos4210 pin-controller 2 */ @@ -629,10 +650,10 @@ static struct samsung_pin_bank exynos4x12_pin_banks1[] = { EXYNOS_PIN_BANK_EINTN(8, 0x1A0, gpy4), EXYNOS_PIN_BANK_EINTN(8, 0x1C0, gpy5), EXYNOS_PIN_BANK_EINTN(8, 0x1E0, gpy6), - EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00), - EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04), - EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08), - EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c), + EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00, 0), + EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04, 8), + EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08, 16), + EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c, 24), }; /* pin banks of exynos4x12 pin-controller 2 */ @@ -724,10 +745,10 @@ static struct samsung_pin_bank exynos5250_pin_banks0[] = { EXYNOS_PIN_BANK_EINTN(8, 0x220, gpy4), EXYNOS_PIN_BANK_EINTN(8, 0x240, gpy5), EXYNOS_PIN_BANK_EINTN(8, 0x260, gpy6), - EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00), - EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04), - EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08), - EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c), + EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00, 0), + EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04, 8), + EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08, 16), + EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c, 24), }; /* pin banks of exynos5250 pin-controller 1 */ diff --git a/drivers/pinctrl/pinctrl-exynos.h b/drivers/pinctrl/pinctrl-exynos.h index 9b1f77a..d98e9ff 100644 --- a/drivers/pinctrl/pinctrl-exynos.h +++ b/drivers/pinctrl/pinctrl-exynos.h @@ -65,13 +65,14 @@ .name = id\ } -#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs) \ +#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs, base)\ { \ .type = bank_type_alive, \ .pctl_offset= reg, \ .nr_pins= pins, \ .eint_type = EINT_TYPE_WKUP, \ .eint_offset= offs, \ + .eint_base = base, \ .name = id\ } diff --git a/drivers/pinctrl/pinctrl-samsung.h b/drivers/pinctrl/pinctrl-samsung.h
Re: Pulls and drive strengths in the pinctrl world
Tomasz / Stephen, On Wed, May 15, 2013 at 5:55 PM, Doug Anderson diand...@google.com wrote: Also after reading Stephen's reply, I'm wondering if hogging wouldn't solve the problem indeed. (It might have to be fixed on pinctrl-samsung first, as last time I tried to use it, it caused some errors from pinctrl core, but haven't time to track them down, as it wasn't anything important at that time). I will give it a shot tomorrow morning and see how it looks. I'll also evaluate Stephen's suggestions then once I've see how it looks with the current bindings... I wrote this out and it had some nice properties (it was a little more concise), but had the downside that there's no reference from the GPIO usage back to the pinmux. I'd rather have the reference there in the hopes that it will help others get things right when they make changes to the dts file (they'll notice the reference and know that they need to change that too). ...so I think the summary is: I'm OK with keeping what we have. It may be a little awkward in some ways but it's definitely worth it to get all of the benefits of the pinmux / GPIO separation. :) For the curious of what my prototype looked like (feel free to ignore--I'm not planning on keeping this and I didn't actually try testing it), I've included it below. This is just the bit from cros5250-common (the common file shared among several similar boards), so I'd need something similar in exynos5250-snow). pinctrl@1140 { /* Default states for hogs follow */ nopull_inputs_cros5250_a: nopull-inputs-cros5250-a { samsung,pins = gpx1-2, /* trackpad */ gpx1-3, /* gpio-keys - power */ gpx3-2; /* max77686 */ samsung,pin-function = 0; samsung,pin-pud = 0; samsung,pin-drv = 0; }; pulldown_inputs_cros5250-a: pulldown-inputs-cros5250_a { samsung,pins = gpx3-7; /* hdmi */ samsung,pin-function = 0; samsung,pin-pud = 1; samsung,pin-drv = 0; }; simple_outputs_cros5250-a: simple-outputs-cros5250_a { samsung,pins = gpx0-1, /* wifi-en */ gpx0-2, /* wifi-rst */ gpx1-7; /* max98095-en */ samsung,pin-function = 1; samsung,pin-pud = 0; samsung,pin-drv = 0; } pinctrl-names = default; pinctrl-0 = nopull_inputs_cros5250_a pulldown_inputs_cros5250_a simple_outputs_cros5250_a; }; pinctrl@1340 { simple_outputs_cros5250-b: simple-outputs-cros5250_b { samsung,pins = gpe1-0 /* hsic reset */; samsung,pin-function = 1; samsung,pin-pud = 0; samsung,pin-drv = 0; }; pinctrl-names = default; pinctrl-0 = simple_outputs_cros5250_b; }; -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
Hi Doug, Thanks for the patch. See my comments inline. On Thursday 16 of May 2013 10:12:31 Doug Anderson wrote: The GPIO states need to be restored after s2r and this is not currently supported in the pinctrl driver. This patch saves the gpio states before suspend and restores them after resume. Logic and commenting for samsung_pinctrl_resume_noirq() is heavily borrowed from the old samsung_gpio_pm_2bit_resume(), which seemed to do this a reasonable way. Patch originally from Prathyush K prathyus...@samsung.com but rewritten by Doug Anderson diand...@chromium.org. Signed-off-by: Prathyush K prathyus...@samsung.com Signed-off-by: Doug Anderson diand...@chromium.org --- drivers/pinctrl/pinctrl-samsung.c | 199 ++ drivers/pinctrl/pinctrl-samsung.h | 11 +++ 2 files changed, 210 insertions(+) diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c index 9763668..0891667 100644 --- a/drivers/pinctrl/pinctrl-samsung.c +++ b/drivers/pinctrl/pinctrl-samsung.c @@ -964,6 +964,204 @@ static int samsung_pinctrl_probe(struct platform_device *pdev) return 0; } +#ifdef CONFIG_PM + +/** + * samsung_pinctrl_resume_noirq - save pinctrl state for suspend + * + * Save data for all banks handled by this device. + */ +static int samsung_pinctrl_suspend_noirq(struct device *dev) +{ + struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev); + struct samsung_pin_ctrl *ctrl = drvdata-ctrl; + void __iomem * const virt_base = drvdata-virt_base; Nit: This const is ugly :) . Is it needed for anything? + int i; + + for (i = 0; i ctrl-nr_banks; i++) { + struct samsung_pin_bank *bank = ctrl-pin_banks[i]; + const struct samsung_pin_bank_type *type = bank-type; + void __iomem * const reg = virt_base + bank-pctl_offset; Nit: This one is not pretty either. + bank-pm_save.con = readl(reg + + type- reg_offset[PINCFG_TYPE_FUNC]); + if (type-fld_width[PINCFG_TYPE_FUNC] 4) What is this condition supposed to check? + bank-pm_save.con |= (u64)readl(reg + 4 + + type-reg_offset[PINCFG_TYPE_FUNC]) 32; This looks ugly. Whatever is going on here, wouldn't it be better to use separate field, like con2 or something? + bank-pm_save.dat = readl(reg + + type- reg_offset[PINCFG_TYPE_DAT]); + bank-pm_save.pud = readl(reg + + type- reg_offset[PINCFG_TYPE_PUD]); + bank-pm_save.drv = readl(reg + + type- reg_offset[PINCFG_TYPE_DRV]); + + if (type-fld_width[PINCFG_TYPE_CON_PDN]) { + bank-pm_save.conpdn = readl(reg + + type-reg_offset[PINCFG_TYPE_CON_PDN]); + bank-pm_save.pudpdn = readl(reg + + type-reg_offset[PINCFG_TYPE_PUD_PDN]); + } I wonder if you couldn't do all the saving here in a single loop over all pin control types, like: unsigned int offsets = bank-type-reg_offsets; unsigned int widths = bank-type-fld_width; for (i = 0; i PINCFG_TYPE_NUM; ++i) if (widths[i]) bank-pm_save[i] = readl(reg + offsets[i]); The only thing not handled by this loop is second CON registers in banks with two of them. I can't think of any better solution for this other than just adding a special case after the loop. + dev_dbg(dev, Save %s @ %p (con %#010llx)\n, + bank-name, reg, bank-pm_save.con); + } + + return 0; +} + +/** + * is_sfn - test whether a pin config represents special function. + * + * Test whether the given masked+shifted bits of an GPIO configuration + * are one of the SFN (special function) modes. + */ +static inline int is_sfn(u32 con) +{ + return con = 2; +} + +/** + * is_in - test if the given masked+shifted GPIO configuration is an input. + */ +static inline int is_in(u32 con) +{ + return con == 0; +} + +/** + * is_out - test if the given masked+shifted GPIO configuration is an output. + */ +static inline int is_out(u32 con) +{ + return con == 1; +} + +/** + * samsung_pinctrl_resume_noirq - restore pinctrl state from suspend + * + * Restore one of the GPIO banks that was saved during suspend. This is + * not as simple as once thought, due to the possibility of glitches + * from the order that the CON and DAT registers are set in. + * + * The three states the pin can be are {IN,OUT,SFN} which gives us 9 + * combinations of changes to check. Three of these, if the pin stays + * in the same configuration can be discounted. This leaves us with + * the following: + * + * { IN = OUT } Change DAT first + * { IN
Re: [PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()
On Thursday 16 of May 2013 10:12:32 Doug Anderson wrote: From: Prathyush K prathyus...@samsung.com Add the irq_set_wake function for exynos pinctrl to configure the external interrupt wakeup mask register. [dianders: minor nit fixes; port to ToT] Signed-off-by: Prathyush K prathyus...@samsung.com Signed-off-by: Doug Anderson diand...@chromium.org --- drivers/pinctrl/pinctrl-exynos.c | 45 --- drivers/pinctrl/pinctrl-exynos.h | 3 ++- drivers/pinctrl/pinctrl-samsung.h | 2 ++ 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c index ac74281..3ebb2ff 100644 --- a/drivers/pinctrl/pinctrl-exynos.c +++ b/drivers/pinctrl/pinctrl-exynos.c @@ -30,6 +30,8 @@ #include linux/spinlock.h #include linux/err.h +#include plat/pm.h + This is not going to work with CONFIG_MULTIPLATFORM. Now this raises a question what is the preferred way to pass some data from generic driver to platform code. I would suggest adding a function called exynos_pinctrl_get_eintmask() (or whatever) that would return the wake-up mask configured in the driver and then modify platform code to use it. #include pinctrl-samsung.h #include pinctrl-exynos.h @@ -326,6 +328,24 @@ static int exynos_wkup_irq_set_type(struct irq_data *irqd, unsigned int type) return 0; } +static int exynos_wkup_irq_set_wake(struct irq_data *irqd, unsigned int state) +{ + struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd); + const int eint_num = bank-eint_base + irqd-hwirq; + unsigned long bit = 1L eint_num; + + pr_info(wake %s for eint %d / %s[%ld]\n, + state ? enabled : disabled, + eint_num, bank-name, irqd-hwirq); + + if (!state) + s3c_irqwake_eintmask |= bit; + else + s3c_irqwake_eintmask = ~bit; + + return 0; +} + /* * irq_chip for wakeup interrupts */ @@ -335,6 +355,7 @@ static struct irq_chip exynos_wkup_irq_chip = { .irq_mask = exynos_wkup_irq_mask, .irq_ack= exynos_wkup_irq_ack, .irq_set_type = exynos_wkup_irq_set_type, + .irq_set_wake = exynos_wkup_irq_set_wake, }; /* interrupt handler for wakeup interrupts 0..15 */ @@ -543,10 +564,10 @@ static struct samsung_pin_bank exynos4210_pin_banks1[] = { EXYNOS_PIN_BANK_EINTN(8, 0x1A0, gpy4), EXYNOS_PIN_BANK_EINTN(8, 0x1C0, gpy5), EXYNOS_PIN_BANK_EINTN(8, 0x1E0, gpy6), - EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00), - EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04), - EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08), - EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c), + EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00, 0), + EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04, 8), + EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08, 16), + EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c, 24), }; /* pin banks of exynos4210 pin-controller 2 */ @@ -629,10 +650,10 @@ static struct samsung_pin_bank exynos4x12_pin_banks1[] = { EXYNOS_PIN_BANK_EINTN(8, 0x1A0, gpy4), EXYNOS_PIN_BANK_EINTN(8, 0x1C0, gpy5), EXYNOS_PIN_BANK_EINTN(8, 0x1E0, gpy6), - EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00), - EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04), - EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08), - EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c), + EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00, 0), + EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04, 8), + EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08, 16), + EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c, 24), }; /* pin banks of exynos4x12 pin-controller 2 */ @@ -724,10 +745,10 @@ static struct samsung_pin_bank exynos5250_pin_banks0[] = { EXYNOS_PIN_BANK_EINTN(8, 0x220, gpy4), EXYNOS_PIN_BANK_EINTN(8, 0x240, gpy5), EXYNOS_PIN_BANK_EINTN(8, 0x260, gpy6), - EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00), - EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04), - EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08), - EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c), + EXYNOS_PIN_BANK_EINTW(8, 0xC00, gpx0, 0x00, 0), + EXYNOS_PIN_BANK_EINTW(8, 0xC20, gpx1, 0x04, 8), + EXYNOS_PIN_BANK_EINTW(8, 0xC40, gpx2, 0x08, 16), + EXYNOS_PIN_BANK_EINTW(8, 0xC60, gpx3, 0x0c, 24), }; /* pin banks of exynos5250 pin-controller 1 */ diff --git a/drivers/pinctrl/pinctrl-exynos.h b/drivers/pinctrl/pinctrl-exynos.h index 9b1f77a..d98e9ff 100644 --- a/drivers/pinctrl/pinctrl-exynos.h +++ b/drivers/pinctrl/pinctrl-exynos.h @@ -65,13 +65,14 @@ .name = id\ } -#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs) \ +#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs, base)\ { \ .type = bank_type_alive, \ .pctl_offset= reg,
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
Tomasz, Thanks for the review! I'll get a new patch out either today or tomorrow... On Thu, May 16, 2013 at 12:19 PM, Tomasz Figa tomasz.f...@gmail.com wrote: +/** + * samsung_pinctrl_resume_noirq - save pinctrl state for suspend + * + * Save data for all banks handled by this device. + */ +static int samsung_pinctrl_suspend_noirq(struct device *dev) +{ + struct samsung_pinctrl_drv_data *drvdata = dev_get_drvdata(dev); + struct samsung_pin_ctrl *ctrl = drvdata-ctrl; + void __iomem * const virt_base = drvdata-virt_base; Nit: This const is ugly :) . Is it needed for anything? Not anything really. I just got in the habit of adding them for variables that are simple shorthand variables: AKA I'm only creating this variable to avoid typing some long complicated thing below. It's a hint to someone reading the code that they don't need to think about it. I have also occasionally caught bugs by doing this. ...but I can understand the dislike. I'll remove this and other similar (but keep const pointers). + if (type-fld_width[PINCFG_TYPE_FUNC] 4) What is this condition supposed to check? It is supposed to be checking whether there are two CON registers in use. ...oh, but that's probably not the right way to do it now that I think about it. I need to check (bank-nr_pins * type-fld_width[PINCFG_TYPE_FUNC]). I will fix. + bank-pm_save.con |= (u64)readl(reg + 4 + + type-reg_offset[PINCFG_TYPE_FUNC]) 32; This looks ugly. Whatever is going on here, wouldn't it be better to use separate field, like con2 or something? Probably. The resume code seemed cleaner with a 64-bit value, but I think I could make it nearly as clean with two 32-bit ones by using a helper function. I wonder if you couldn't do all the saving here in a single loop over all pin control types, like: unsigned int offsets = bank-type-reg_offsets; unsigned int widths = bank-type-fld_width; for (i = 0; i PINCFG_TYPE_NUM; ++i) if (widths[i]) bank-pm_save[i] = readl(reg + offsets[i]); The only thing not handled by this loop is second CON registers in banks with two of them. I can't think of any better solution for this other than just adding a special case after the loop. Yes, that would work. I think it wasn't possible when I first wrote the code against an older code base that didn't have the arrays. I can give it a shot if it doesn't make restore too bad... Now as I think of it, do CON_PDN and PUD_PDN really need to be saved? I couldn't find in the documentation if they are preserved or lost in sleep mode. Do you have some information on this? I remember it being important. Running a test now. Yes, it's important on exynos5250. As an example: [ 62.22] samsung-pinctrl 1140.pinctrl: Restore gpa2@f0048040 (0x=0x2212 ch 0x) [ 62.22] samsung-pinctrl 1140.pinctrl: Restore gpb0@f0048060 CON_PDN (0x=0x02aa) [ 62.22] samsung-pinctrl 1140.pinctrl: Restore gpb0@f0048060 PUD_PDN (0x=0x0155) I wonder if the whole restoration procedure couldn't be simplified. I don't remember my version being so complicated, but I don't have my patch on my screen at the moment, so I might be wrong on this. I debated about this a bunch. Perhaps I should just delete it. I saw that it was there in the old 2-bit code and it also seemed quite reasonable, so I kept it. Things seem to work OK without it, but most things are pretty tolerant to their lines glitching (or even driving high to low for a short period of time). ...but your question made me check again. From previous experimentation I'm pretty certain that most pins on the exynos are held in the powerdown state even during early bootup of the SoC. The hope is that they are released from powerdown _after_ the GPIO init is called. If not then we're already glitching somewhat as we transition from powerdown state to default state before this function finally gets called. Looking at exynos, that's probably done in exynos_pm_resume(), maybe by mucking with the pad retention options? Oh, that probably means taht no_irq() is too late and that I need to figure out how to get my code called earlier... The exynos_pm_resume() is called by syscore. +#else +#define samsung_pinctrl_suspend_noirqNULL +#define samsung_pinctrl_resume_noirq NULL +#endif + +static const struct dev_pm_ops samsung_pinctrl_dev_pm_ops = { + .suspend_noirq = samsung_pinctrl_suspend_noirq, + .resume_noirq = samsung_pinctrl_resume_noirq, +}; I'm not sure if resume_noirq is really early enough. Some drivers might already need correct pin configuration in their resume_noirq callback. In my patch I have used syscore_ops to register very late suspend and very early resume callbacks for the whole pinctrl-samsung driver and a global list of registered pin
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
Am Donnerstag, 16. Mai 2013, 21:19:20 schrieb Tomasz Figa: [...] + + if (type-fld_width[PINCFG_TYPE_CON_PDN]) { + bank-pm_save.conpdn = readl(reg + + type-reg_offset[PINCFG_TYPE_CON_PDN]); + bank-pm_save.pudpdn = readl(reg + + type-reg_offset[PINCFG_TYPE_PUD_PDN]); + } I wonder if you couldn't do all the saving here in a single loop over all pin control types, like: unsigned int offsets = bank-type-reg_offsets; unsigned int widths = bank-type-fld_width; for (i = 0; i PINCFG_TYPE_NUM; ++i) if (widths[i]) bank-pm_save[i] = readl(reg + offsets[i]); The only thing not handled by this loop is second CON registers in banks with two of them. I can't think of any better solution for this other than just adding a special case after the loop. doing this in the loop over the pinctrl types like Tomasz suggests, also nicely fixes the problem of s3c24xx [0] only having FUNC, DAT and PUD and some (gpa) not even having the PUD, which was not checked in the original patch. Heiko [0] patch is with Kgene currently, so should make it into 3.11 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
On Thursday 16 of May 2013 13:32:48 Doug Anderson wrote: Tomasz, Thanks for the review! I'll get a new patch out either today or tomorrow... OK. I will be fine to go with your patches, after addressing the comments. In the end it's good that you posted them, as reviewing them allowed me to find even better ways of doing some things than I had in mine ;) . [snip] I wonder if you couldn't do all the saving here in a single loop over all pin control types, like: unsigned int offsets = bank-type-reg_offsets; unsigned int widths = bank-type-fld_width; for (i = 0; i PINCFG_TYPE_NUM; ++i) if (widths[i]) bank-pm_save[i] = readl(reg + offsets[i]); The only thing not handled by this loop is second CON registers in banks with two of them. I can't think of any better solution for this other than just adding a special case after the loop. Yes, that would work. I think it wasn't possible when I first wrote the code against an older code base that didn't have the arrays. I can give it a shot if it doesn't make restore too bad... OK. I wonder if resume couldn't somehow benefit from this as well, but it probably depends on how much it can be altered without breaking the anti- glitch functionality. Now as I think of it, do CON_PDN and PUD_PDN really need to be saved? I couldn't find in the documentation if they are preserved or lost in sleep mode. Do you have some information on this? I remember it being important. Running a test now. Yes, it's important on exynos5250. As an example: [ 62.22] samsung-pinctrl 1140.pinctrl: Restore gpa2@f0048040 (0x=0x2212 ch 0x) [ 62.22] samsung-pinctrl 1140.pinctrl: Restore gpb0@f0048060 CON_PDN (0x=0x02aa) [ 62.22] samsung-pinctrl 1140.pinctrl: Restore gpb0@f0048060 PUD_PDN (0x=0x0155) OK. It's good to know. I wonder if the whole restoration procedure couldn't be simplified. I don't remember my version being so complicated, but I don't have my patch on my screen at the moment, so I might be wrong on this. I debated about this a bunch. Perhaps I should just delete it. I saw that it was there in the old 2-bit code and it also seemed quite reasonable, so I kept it. Things seem to work OK without it, but most things are pretty tolerant to their lines glitching (or even driving high to low for a short period of time). ...but your question made me check again. From previous experimentation I'm pretty certain that most pins on the exynos are held in the powerdown state even during early bootup of the SoC. The hope is that they are released from powerdown _after_ the GPIO init is called. If not then we're already glitching somewhat as we transition from powerdown state to default state before this function finally gets called. Looking at exynos, that's probably done in exynos_pm_resume(), maybe by mucking with the pad retention options? Oh, that probably means taht no_irq() is too late and that I need to figure out how to get my code called earlier... The exynos_pm_resume() is called by syscore. How all of this works is basically a good question. I couldn't find any mention about pins switching from power down to normal mode in the documentation, but maybe there is a small side note somewhere, which I could miss. On S3C6410, for example, there are two modes. State is switched to power down mode automatically, but can be switched out either automatically on wake-up (exact timing is unknown to me) or by clearing a special bit, depending on value of other special bit. IMHO this is rather important, so we should find out how it work on other SoCs and make the code account for it. Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
Tomasz, On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa tomasz.f...@gmail.com wrote: OK. I will be fine to go with your patches, after addressing the comments. In the end it's good that you posted them, as reviewing them allowed me to find even better ways of doing some things than I had in mine ;) . Yes. I often find that the best way to review code is to think about how I would implement it myself. Certainly I think we've ended up with something better / less buggy this way. ;) How all of this works is basically a good question. I couldn't find any mention about pins switching from power down to normal mode in the documentation, but maybe there is a small side note somewhere, which I could miss. On S3C6410, for example, there are two modes. State is switched to power down mode automatically, but can be switched out either automatically on wake-up (exact timing is unknown to me) or by clearing a special bit, depending on value of other special bit. IMHO this is rather important, so we should find out how it work on other SoCs and make the code account for it. Agreed that it's important. ...but it's also good not to have tons of complexity when it's not needed. It sounds like S3C6410 could be handled OK by just using the special bits and waiting to take things out of power down mode. ...thinking about it, all SoCs that have power down modes (which you _must_ have if your pinctrl state is lost across a low power) would be slightly broken if they didn't have a bit to switch out of power down mode. Otherwise you're asking for at least some type of glitch because you'll end up in the default state of pins for a little while during resume. That's not to say that there aren't broken SoCs out there and it's entirely possible that people even designed systems around them (knowing that the default state of each pin after wakeup is not harmful to whatever is connected to that pin). If there are any cases like this then they would need the special code like my V1 patch had. Do you know of any SoCs like this that we need to support on kernel 3.10 and higher? I'm planning on going back to the simpler code for my next patchset unless I can find a problem with it. -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
On Thursday 16 of May 2013 14:51:53 Doug Anderson wrote: Tomasz, On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa tomasz.f...@gmail.com wrote: OK. I will be fine to go with your patches, after addressing the comments. In the end it's good that you posted them, as reviewing them allowed me to find even better ways of doing some things than I had in mine ;) . Yes. I often find that the best way to review code is to think about how I would implement it myself. Certainly I think we've ended up with something better / less buggy this way. ;) How all of this works is basically a good question. I couldn't find any mention about pins switching from power down to normal mode in the documentation, but maybe there is a small side note somewhere, which I could miss. On S3C6410, for example, there are two modes. State is switched to power down mode automatically, but can be switched out either automatically on wake-up (exact timing is unknown to me) or by clearing a special bit, depending on value of other special bit. IMHO this is rather important, so we should find out how it work on other SoCs and make the code account for it. Agreed that it's important. ...but it's also good not to have tons of complexity when it's not needed. It sounds like S3C6410 could be handled OK by just using the special bits and waiting to take things out of power down mode. ...thinking about it, all SoCs that have power down modes (which you _must_ have if your pinctrl state is lost across a low power) would be slightly broken if they didn't have a bit to switch out of power down mode. Otherwise you're asking for at least some type of glitch because you'll end up in the default state of pins for a little while during resume. That's not to say that there aren't broken SoCs out there and it's entirely possible that people even designed systems around them (knowing that the default state of each pin after wakeup is not harmful to whatever is connected to that pin). If there are any cases like this then they would need the special code like my V1 patch had. Do you know of any SoCs like this that we need to support on kernel 3.10 and higher? Hmm, I just checked documentation of S3C2440 and S3C2416 they seem to retain GPIO settings completely in sleep mode. This would mean that they don't require any suspend/resume support in pinctrl driver. Heiko, can you confirm this? Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()
Tomasz, On Thu, May 16, 2013 at 12:26 PM, Tomasz Figa tomasz.f...@gmail.com wrote: On Thursday 16 of May 2013 10:12:32 Doug Anderson wrote: From: Prathyush K prathyus...@samsung.com Add the irq_set_wake function for exynos pinctrl to configure the external interrupt wakeup mask register. [dianders: minor nit fixes; port to ToT] Signed-off-by: Prathyush K prathyus...@samsung.com Signed-off-by: Doug Anderson diand...@chromium.org --- drivers/pinctrl/pinctrl-exynos.c | 45 --- drivers/pinctrl/pinctrl-exynos.h | 3 ++- drivers/pinctrl/pinctrl-samsung.h | 2 ++ 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c index ac74281..3ebb2ff 100644 --- a/drivers/pinctrl/pinctrl-exynos.c +++ b/drivers/pinctrl/pinctrl-exynos.c @@ -30,6 +30,8 @@ #include linux/spinlock.h #include linux/err.h +#include plat/pm.h + This is not going to work with CONFIG_MULTIPLATFORM. Hmm, this sounds like it might be a bit of a long path, especially since I haven't been keeping up with what's been going on with MULTIPLATFORM and I'm currently midway through making 3.8 work (which has no MULTIPLATFORM). Perhaps for this patch it makes more sense for you to post your version and I can review it? We may end up just keeping our version of this patch for 3.8 and pick up yours when we do our next rebase. Does that sound OK? -#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs) \ +#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs, base)\ { \ .type = bank_type_alive, \ .pctl_offset= reg, \ .nr_pins= pins, \ .eint_type = EINT_TYPE_WKUP, \ .eint_offset= offs, \ + .eint_base = base, \ I can't look at my patch at the moment, but I think I have managed to get EINT index without adding this extra field. It looks like this is always 2 * eint_offset in the code above. Maybe you just multiplied? The multiplication works fine although I think specifying eint_base like this might be more generic and handle future chips better? Ya never know... -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
Am Freitag, 17. Mai 2013, 00:08:34 schrieb Tomasz Figa: On Thursday 16 of May 2013 14:51:53 Doug Anderson wrote: Tomasz, On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa tomasz.f...@gmail.com wrote: OK. I will be fine to go with your patches, after addressing the comments. In the end it's good that you posted them, as reviewing them allowed me to find even better ways of doing some things than I had in mine ;) . Yes. I often find that the best way to review code is to think about how I would implement it myself. Certainly I think we've ended up with something better / less buggy this way. ;) How all of this works is basically a good question. I couldn't find any mention about pins switching from power down to normal mode in the documentation, but maybe there is a small side note somewhere, which I could miss. On S3C6410, for example, there are two modes. State is switched to power down mode automatically, but can be switched out either automatically on wake-up (exact timing is unknown to me) or by clearing a special bit, depending on value of other special bit. IMHO this is rather important, so we should find out how it work on other SoCs and make the code account for it. Agreed that it's important. ...but it's also good not to have tons of complexity when it's not needed. It sounds like S3C6410 could be handled OK by just using the special bits and waiting to take things out of power down mode. ...thinking about it, all SoCs that have power down modes (which you _must_ have if your pinctrl state is lost across a low power) would be slightly broken if they didn't have a bit to switch out of power down mode. Otherwise you're asking for at least some type of glitch because you'll end up in the default state of pins for a little while during resume. That's not to say that there aren't broken SoCs out there and it's entirely possible that people even designed systems around them (knowing that the default state of each pin after wakeup is not harmful to whatever is connected to that pin). If there are any cases like this then they would need the special code like my V1 patch had. Do you know of any SoCs like this that we need to support on kernel 3.10 and higher? Hmm, I just checked documentation of S3C2440 and S3C2416 they seem to retain GPIO settings completely in sleep mode. This would mean that they don't require any suspend/resume support in pinctrl driver. Heiko, can you confirm this? Hmm, my system does not have a working suspend right now, but looking at the legacy code (mach-s3c24xx/pm.c, etc) tells me that the gpio banks were never saved during suspend. And as there were (and still are) systems with working suspend around, I'd assume that you're correct that the pins retain their state. Is the same true for the s3c64xx, as I didn't find any gpio suspend handling for it either. Heiko -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()
On Thursday 16 of May 2013 15:25:15 Doug Anderson wrote: Tomasz, On Thu, May 16, 2013 at 12:26 PM, Tomasz Figa tomasz.f...@gmail.com wrote: On Thursday 16 of May 2013 10:12:32 Doug Anderson wrote: From: Prathyush K prathyus...@samsung.com Add the irq_set_wake function for exynos pinctrl to configure the external interrupt wakeup mask register. [dianders: minor nit fixes; port to ToT] Signed-off-by: Prathyush K prathyus...@samsung.com Signed-off-by: Doug Anderson diand...@chromium.org --- drivers/pinctrl/pinctrl-exynos.c | 45 --- drivers/pinctrl/pinctrl-exynos.h | 3 ++- drivers/pinctrl/pinctrl-samsung.h | 2 ++ 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c index ac74281..3ebb2ff 100644 --- a/drivers/pinctrl/pinctrl-exynos.c +++ b/drivers/pinctrl/pinctrl-exynos.c @@ -30,6 +30,8 @@ #include linux/spinlock.h #include linux/err.h +#include plat/pm.h + This is not going to work with CONFIG_MULTIPLATFORM. Hmm, this sounds like it might be a bit of a long path, especially since I haven't been keeping up with what's been going on with MULTIPLATFORM and I'm currently midway through making 3.8 work (which has no MULTIPLATFORM). Well, to make long story short, including headers from plat/ and mach/ from files outside plat/ or mach/ is no longer valid with CONFIG_MULTIPLATFORM, because more than one plat and/or mach can be enabled at the same time. In addition to this, care must be taken for code to not break platforms other than written for, when compiled into the resulting kernel. Perhaps for this patch it makes more sense for you to post your version and I can review it? We may end up just keeping our version of this patch for 3.8 and pick up yours when we do our next rebase. Does that sound OK? Fine. I will also send a patch adding save and restore for several EINT registers that need it. -#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs) \ +#define EXYNOS_PIN_BANK_EINTW(pins, reg, id, offs, base)\ { \ .type = bank_type_alive, \ .pctl_offset= reg, \ .nr_pins= pins, \ .eint_type = EINT_TYPE_WKUP, \ .eint_offset= offs, \ + .eint_base = base, \ I can't look at my patch at the moment, but I think I have managed to get EINT index without adding this extra field. It looks like this is always 2 * eint_offset in the code above. Maybe you just multiplied? The multiplication works fine although I think specifying eint_base like this might be more generic and handle future chips better? Ya never know... Since EINT handling is highly SoC-specific (i.e. done in pinctrl-exynos, not pinctrl-samsung), such assumption wouldn't be a problem. Let me see how I solved this problem in my version tomorrow at work. Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
On Friday 17 of May 2013 00:30:38 Heiko Stübner wrote: Am Freitag, 17. Mai 2013, 00:08:34 schrieb Tomasz Figa: On Thursday 16 of May 2013 14:51:53 Doug Anderson wrote: Tomasz, On Thu, May 16, 2013 at 2:27 PM, Tomasz Figa tomasz.f...@gmail.com wrote: OK. I will be fine to go with your patches, after addressing the comments. In the end it's good that you posted them, as reviewing them allowed me to find even better ways of doing some things than I had in mine ;) . Yes. I often find that the best way to review code is to think about how I would implement it myself. Certainly I think we've ended up with something better / less buggy this way. ;) How all of this works is basically a good question. I couldn't find any mention about pins switching from power down to normal mode in the documentation, but maybe there is a small side note somewhere, which I could miss. On S3C6410, for example, there are two modes. State is switched to power down mode automatically, but can be switched out either automatically on wake-up (exact timing is unknown to me) or by clearing a special bit, depending on value of other special bit. IMHO this is rather important, so we should find out how it work on other SoCs and make the code account for it. Agreed that it's important. ...but it's also good not to have tons of complexity when it's not needed. It sounds like S3C6410 could be handled OK by just using the special bits and waiting to take things out of power down mode. ...thinking about it, all SoCs that have power down modes (which you _must_ have if your pinctrl state is lost across a low power) would be slightly broken if they didn't have a bit to switch out of power down mode. Otherwise you're asking for at least some type of glitch because you'll end up in the default state of pins for a little while during resume. That's not to say that there aren't broken SoCs out there and it's entirely possible that people even designed systems around them (knowing that the default state of each pin after wakeup is not harmful to whatever is connected to that pin). If there are any cases like this then they would need the special code like my V1 patch had. Do you know of any SoCs like this that we need to support on kernel 3.10 and higher? Hmm, I just checked documentation of S3C2440 and S3C2416 they seem to retain GPIO settings completely in sleep mode. This would mean that they don't require any suspend/resume support in pinctrl driver. Heiko, can you confirm this? Hmm, my system does not have a working suspend right now, but looking at the legacy code (mach-s3c24xx/pm.c, etc) tells me that the gpio banks were never saved during suspend. And as there were (and still are) systems with working suspend around, I'd assume that you're correct that the pins retain their state. Is the same true for the s3c64xx, as I didn't find any gpio suspend handling for it either. Seems like I need some sleep, as I'm already starting to overlook large blobs of code. Originally, GPIO suspend/resume handlers have been configured in drivers/gpio/gpio-samsung.c, by setting pm field of samsung_gpio_chip struct to point to appropriate samsung_gpio_pm struct, which contains pointers to save and resume callbacks. In result, samsung_gpio_pm_2bit_* or samsung_gpio_pm_4bit_* have been used, depending on bank type, on all SoCs. Now since the documentation states that wake-up reset doesn't reset GPIO registers (at least on S3C2440 and S3C2416), I wonder what is the correct way of handling them. Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
Tomasz, On Thu, May 16, 2013 at 3:56 PM, Tomasz Figa tomasz.f...@gmail.com wrote: Seems like I need some sleep, as I'm already starting to overlook large blobs of code. Originally, GPIO suspend/resume handlers have been configured in drivers/gpio/gpio-samsung.c, by setting pm field of samsung_gpio_chip struct to point to appropriate samsung_gpio_pm struct, which contains pointers to save and resume callbacks. In result, samsung_gpio_pm_2bit_* or samsung_gpio_pm_4bit_* have been used, depending on bank type, on all SoCs. Now since the documentation states that wake-up reset doesn't reset GPIO registers (at least on S3C2440 and S3C2416), I wonder what is the correct way of handling them. If state of these registers isn't lost on those SoCs then running the save/restore shouldn't _hurt_ though, right? If you can run the old GPIO code on one of those systems and do a suspend/resume you could check... -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] pinctrl: samsung: fix suspend/resume functionality
The GPIO states need to be restored after s2r and this is not currently supported in the pinctrl driver. This patch saves the gpio states before suspend and restores them after resume. Saving and restoring is done very early using syscore_ops and must happen before pins are released from their powerdown state. Patch originally from Prathyush K prathyus...@samsung.com but rewritten by Doug Anderson diand...@chromium.org. Signed-off-by: Prathyush K prathyus...@samsung.com Signed-off-by: Doug Anderson diand...@chromium.org --- Changes in v2: - Now uses sycore_ops to make sure we're early enough. - Try to handle two CON registers better. - Should handle s3c24xx better as per Heiko. - Simpler code; no longer tries to avoid glitching lines since we _think_ all current SoCs should have pins in power down state when the restore is called. - Dropped eint patch for now; Tomasz will post his version. drivers/pinctrl/pinctrl-samsung.c | 140 ++ drivers/pinctrl/pinctrl-samsung.h | 5 ++ 2 files changed, 145 insertions(+) diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c index 9763668..851eada 100644 --- a/drivers/pinctrl/pinctrl-samsung.c +++ b/drivers/pinctrl/pinctrl-samsung.c @@ -28,6 +28,7 @@ #include linux/gpio.h #include linux/irqdomain.h #include linux/spinlock.h +#include linux/syscore_ops.h #include core.h #include pinctrl-samsung.h @@ -48,6 +49,9 @@ static struct pin_config { { samsung,pin-pud-pdn, PINCFG_TYPE_PUD_PDN }, }; +/* Global list of devices (struct samsung_pinctrl_drv_data) */ +LIST_HEAD(drvdata_list); + static unsigned int pin_base; static inline struct samsung_pin_bank *gc_to_pin_bank(struct gpio_chip *gc) @@ -961,9 +965,137 @@ static int samsung_pinctrl_probe(struct platform_device *pdev) ctrl-eint_wkup_init(drvdata); platform_set_drvdata(pdev, drvdata); + + /* Add to the global list */ + list_add_tail(drvdata-node, drvdata_list); + return 0; } +#ifdef CONFIG_PM + +/** + * samsung_pinctrl_suspend_dev - save pinctrl state for suspend for a device + * + * Save data for all banks handled by this device. + */ +static void samsung_pinctrl_suspend_dev( + struct samsung_pinctrl_drv_data *drvdata) +{ + struct samsung_pin_ctrl *ctrl = drvdata-ctrl; + void __iomem *virt_base = drvdata-virt_base; + int i; + + for (i = 0; i ctrl-nr_banks; i++) { + struct samsung_pin_bank *bank = ctrl-pin_banks[i]; + void __iomem *reg = virt_base + bank-pctl_offset; + + u8 *offs = bank-type-reg_offset; + u8 *widths = bank-type-fld_width; + enum pincfg_type type; + + for (type = 0; type PINCFG_TYPE_NUM; type++) + if (widths[type]) + bank-pm_save[type] = readl(reg + offs[type]); + + if (widths[PINCFG_TYPE_FUNC] * bank-nr_pins 32) { + /* Some banks have two config registers */ + bank-pm_save[PINCFG_TYPE_NUM] = + readl(reg + offs[PINCFG_TYPE_FUNC] + 4); + pr_debug(Save %s @ %p (con %#010x %08x)\n, +bank-name, reg, +bank-pm_save[PINCFG_TYPE_FUNC], +bank-pm_save[PINCFG_TYPE_NUM]); + } else { + pr_debug(Save %s @ %p (con %#010x)\n, bank-name, +reg, bank-pm_save[PINCFG_TYPE_FUNC]); + } + } +} + +/** + * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a device + * + * Restore one of the banks that was saved during suspend. + * + * We don't bother doing anything complicated to avoid glitching lines since + * we're called before pad retention is turned off. + */ +static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata) +{ + struct samsung_pin_ctrl *ctrl = drvdata-ctrl; + void __iomem *virt_base = drvdata-virt_base; + int i; + + for (i = 0; i ctrl-nr_banks; i++) { + struct samsung_pin_bank *bank = ctrl-pin_banks[i]; + void __iomem *reg = virt_base + bank-pctl_offset; + + u8 *offs = bank-type-reg_offset; + u8 *widths = bank-type-fld_width; + enum pincfg_type type; + + if (widths[PINCFG_TYPE_FUNC] * bank-nr_pins 32) { + /* Some banks have two config registers */ + pr_debug(%s @ %p (con %#010x %08x = %#010x %08x)\n, +bank-name, reg, +readl(reg + offs[PINCFG_TYPE_FUNC]), +readl(reg + offs[PINCFG_TYPE_FUNC] + 4), +bank-pm_save[PINCFG_TYPE_FUNC], +bank-pm_save[PINCFG_TYPE_NUM]); +
Re: [PATCH 2/2] pinctrl: exynos: fix eint wakeup by using irq_set_wake()
Tomasz, On Thu, May 16, 2013 at 3:37 PM, Tomasz Figa tomasz.f...@gmail.com wrote: Well, to make long story short, including headers from plat/ and mach/ from files outside plat/ or mach/ is no longer valid with CONFIG_MULTIPLATFORM, because more than one plat and/or mach can be enabled at the same time. In addition to this, care must be taken for code to not break platforms other than written for, when compiled into the resulting kernel. Right. That makes sense. That's also why it would be a bit of a longshot for me to get this done right now. I'd imagine that there would be a number of changes to the samsung pm infrastructure that are needed to make this work and I don't have all of those in my tree right now. We've already picked back a lot to 3.8, but multiplatform seems too much. Perhaps for this patch it makes more sense for you to post your version and I can review it? We may end up just keeping our version of this patch for 3.8 and pick up yours when we do our next rebase. Does that sound OK? Fine. I will also send a patch adding save and restore for several EINT registers that need it. OK, sounds good. I was trying to figure out why we didn't seem to have those in our 3.4 stuff and that it seems to work without saving/restoring. I assumed that maybe higher level code was masking/unmasking interrupts but didn't dig. Since EINT handling is highly SoC-specific (i.e. done in pinctrl-exynos, not pinctrl-samsung), such assumption wouldn't be a problem. Let me see how I solved this problem in my version tomorrow at work. Fair enough. :) Looking forward to seeing your patch! -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] pinctrl: samsung: fix suspend/resume functionality
Tomasz, On Thu, May 16, 2013 at 4:10 PM, Doug Anderson diand...@chromium.org wrote: If state of these registers isn't lost on those SoCs then running the save/restore shouldn't _hurt_ though, right? If you can run the old GPIO code on one of those systems and do a suspend/resume you could check... I think it's been too long of a day for me, too. I just thought about this and realized that there is no powerdown registers for the GPX banks on exynos5250. ...and they don't lose their state at sleep! ...so maybe a reasonable thing to do would be to skip save/restore in any case where there are no powerdown registers? You can see a printout in my case: [ 412.84] gpx0 @ f004ac00 (con 0x3110 = 0x3110) [ 412.84] gpx1 @ f004ac20 (con 0x1f10ff10 = 0x1f10ff10) [ 412.84] gpx2 @ f004ac40 (con 0x1f000f0f = 0x1f000f0f) [ 412.84] gpx3 @ f004ac60 (con 0x00f00f01 = 0x00f00f01) -Doug -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] pinctrl: samsung: fix suspend/resume functionality
The GPIO states need to be restored after s2r and this is not currently supported in the pinctrl driver. This patch saves the gpio states before suspend and restores them after resume. Saving and restoring is done very early using syscore_ops and must happen before pins are released from their powerdown state. Patch originally from Prathyush K prathyus...@samsung.com but rewritten by Doug Anderson diand...@chromium.org. Signed-off-by: Prathyush K prathyus...@samsung.com Signed-off-by: Doug Anderson diand...@chromium.org --- Changes in v3: - Skip save and restore for banks with no powerdown config. Changes in v2: - Now uses sycore_ops to make sure we're early enough. - Try to handle two CON registers better. - Should handle s3c24xx better as per Heiko. - Simpler code; no longer tries to avoid glitching lines since we _think_ all current SoCs should have pins in power down state when the restore is called. - Dropped eint patch for now; Tomasz will post his version. drivers/pinctrl/pinctrl-samsung.c | 148 ++ drivers/pinctrl/pinctrl-samsung.h | 5 ++ 2 files changed, 153 insertions(+) diff --git a/drivers/pinctrl/pinctrl-samsung.c b/drivers/pinctrl/pinctrl-samsung.c index 9763668..d45e36f 100644 --- a/drivers/pinctrl/pinctrl-samsung.c +++ b/drivers/pinctrl/pinctrl-samsung.c @@ -28,6 +28,7 @@ #include linux/gpio.h #include linux/irqdomain.h #include linux/spinlock.h +#include linux/syscore_ops.h #include core.h #include pinctrl-samsung.h @@ -48,6 +49,9 @@ static struct pin_config { { samsung,pin-pud-pdn, PINCFG_TYPE_PUD_PDN }, }; +/* Global list of devices (struct samsung_pinctrl_drv_data) */ +LIST_HEAD(drvdata_list); + static unsigned int pin_base; static inline struct samsung_pin_bank *gc_to_pin_bank(struct gpio_chip *gc) @@ -961,9 +965,145 @@ static int samsung_pinctrl_probe(struct platform_device *pdev) ctrl-eint_wkup_init(drvdata); platform_set_drvdata(pdev, drvdata); + + /* Add to the global list */ + list_add_tail(drvdata-node, drvdata_list); + return 0; } +#ifdef CONFIG_PM + +/** + * samsung_pinctrl_suspend_dev - save pinctrl state for suspend for a device + * + * Save data for all banks handled by this device. + */ +static void samsung_pinctrl_suspend_dev( + struct samsung_pinctrl_drv_data *drvdata) +{ + struct samsung_pin_ctrl *ctrl = drvdata-ctrl; + void __iomem *virt_base = drvdata-virt_base; + int i; + + for (i = 0; i ctrl-nr_banks; i++) { + struct samsung_pin_bank *bank = ctrl-pin_banks[i]; + void __iomem *reg = virt_base + bank-pctl_offset; + + u8 *offs = bank-type-reg_offset; + u8 *widths = bank-type-fld_width; + enum pincfg_type type; + + /* Registers without a powerdown config aren't lost */ + if (!widths[PINCFG_TYPE_CON_PDN]) + continue; + + for (type = 0; type PINCFG_TYPE_NUM; type++) + if (widths[type]) + bank-pm_save[type] = readl(reg + offs[type]); + + if (widths[PINCFG_TYPE_FUNC] * bank-nr_pins 32) { + /* Some banks have two config registers */ + bank-pm_save[PINCFG_TYPE_NUM] = + readl(reg + offs[PINCFG_TYPE_FUNC] + 4); + pr_debug(Save %s @ %p (con %#010x %08x)\n, +bank-name, reg, +bank-pm_save[PINCFG_TYPE_FUNC], +bank-pm_save[PINCFG_TYPE_NUM]); + } else { + pr_debug(Save %s @ %p (con %#010x)\n, bank-name, +reg, bank-pm_save[PINCFG_TYPE_FUNC]); + } + } +} + +/** + * samsung_pinctrl_resume_dev - restore pinctrl state from suspend for a device + * + * Restore one of the banks that was saved during suspend. + * + * We don't bother doing anything complicated to avoid glitching lines since + * we're called before pad retention is turned off. + */ +static void samsung_pinctrl_resume_dev(struct samsung_pinctrl_drv_data *drvdata) +{ + struct samsung_pin_ctrl *ctrl = drvdata-ctrl; + void __iomem *virt_base = drvdata-virt_base; + int i; + + for (i = 0; i ctrl-nr_banks; i++) { + struct samsung_pin_bank *bank = ctrl-pin_banks[i]; + void __iomem *reg = virt_base + bank-pctl_offset; + + u8 *offs = bank-type-reg_offset; + u8 *widths = bank-type-fld_width; + enum pincfg_type type; + + /* Registers without a powerdown config aren't lost */ + if (!widths[PINCFG_TYPE_CON_PDN]) + continue; + + if (widths[PINCFG_TYPE_FUNC] * bank-nr_pins 32) { + /* Some banks have two config registers */ +