| Message ID | 1403705032-14835-5-git-send-email-pankaj.dubey@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Pankaj, Please see my comments inline. On 25.06.2014 16:03, Pankaj Dubey wrote: > This patch modifies Exynos Power Management Unit (PMU) initialization > implementation in following way: > > - Added platform driver support and probe function where Exynos PMU > driver will register itself as syscon provider with syscon framework. > - Added platform struct exynos_pmu_data to hold platform specific data. > - For each SoC's PMU support now we can add platform data and statically > bind PMU configuration and SoC specific initialization function. > - Separate each SoC's PMU initialization function and make it as part of > platform data. > - It also removes uses of soc_is_exynosXYZ(). [snip] > @@ -93,7 +112,7 @@ static const struct exynos_pmu_conf exynos4210_pmu_config[] = { > { PMU_TABLE_END,}, > }; > > -static const struct exynos_pmu_conf exynos4x12_pmu_config[] = { > +static const struct exynos_pmu_conf exynos4212_pmu_config[] = { Why the name change? > { S5P_ARM_CORE0_LOWPWR, { 0x0, 0x0, 0x2 } }, > { S5P_DIS_IRQ_CORE0, { 0x0, 0x0, 0x0 } }, > { S5P_DIS_IRQ_CENTRAL0, { 0x0, 0x0, 0x0 } }, > @@ -335,7 +354,7 @@ static unsigned int const exynos5_list_diable_wfi_wfe[] = { > EXYNOS5_ISP_ARM_OPTION, > }; > > -static void exynos5_init_pmu(void) > +static void exynos5_powerdown_conf(enum sys_powerdown mode) > { > unsigned int i; > unsigned int tmp; > @@ -372,51 +391,151 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode) > { > unsigned int i; > > - if (soc_is_exynos5250()) > - exynos5_init_pmu(); > + struct exynos_pmu_data *pmu_data = pmu_context->pmu_data; > + > + if (!pmu_data) > + return; > > - for (i = 0; (exynos_pmu_config[i].offset != PMU_TABLE_END) ; i++) > - __raw_writel(exynos_pmu_config[i].val[mode], > - pmu_base_addr + exynos_pmu_config[i].offset); > + if (pmu_data->powerdown_conf) > + pmu_data->powerdown_conf(mode); > > - if (soc_is_exynos4412()) { > - for (i = 0; exynos4412_pmu_config[i].offset != PMU_TABLE_END ; i++) > - __raw_writel(exynos4412_pmu_config[i].val[mode], > - pmu_base_addr + exynos4412_pmu_config[i].offset); > + if (pmu_data->pmu_config) { > + for (i = 0; (pmu_data->pmu_config[i].offset != PMU_TABLE_END) ; i++) > + __raw_writel(pmu_data->pmu_config[i].val[mode], > + pmu_base_addr + pmu_data->pmu_config[i].offset); Analogically to patch 2/5, you could add static inline accessors and use them instead of adding pmu_base_addr every time. > + } > + > + if (pmu_data->pmu_config_extra) { > + for (i = 0; pmu_data->pmu_config_extra[i].offset != PMU_TABLE_END; i++) > + __raw_writel(pmu_data->pmu_config_extra[i].val[mode], > + pmu_base_addr + pmu_data->pmu_config_extra[i].offset); > } > } > > -static int __init exynos_pmu_init(void) > +static void exynos5250_pmu_init(void) > { > unsigned int value; > + /* > + * When SYS_WDTRESET is set, watchdog timer reset request > + * is ignored by power management unit. > + */ > + value = __raw_readl(pmu_base_addr + EXYNOS5_AUTO_WDTRESET_DISABLE); > + value &= ~EXYNOS5_SYS_WDTRESET; > + __raw_writel(value, pmu_base_addr + EXYNOS5_AUTO_WDTRESET_DISABLE); > + > + value = __raw_readl(pmu_base_addr + EXYNOS5_MASK_WDTRESET_REQUEST); > + value &= ~EXYNOS5_SYS_WDTRESET; > + __raw_writel(value, pmu_base_addr + EXYNOS5_MASK_WDTRESET_REQUEST); > +} > + > +static struct exynos_pmu_data exynos4210_pmu_data = { static const struct > + .pmu_config = exynos4210_pmu_config, > +}; > + > +static struct exynos_pmu_data exynos4212_pmu_data = { static const struct > + .pmu_config = exynos4212_pmu_config, > +}; > + > +static struct exynos_pmu_data exynos4412_pmu_data = { static const struct > + .pmu_config = exynos4212_pmu_config, > + .pmu_config_extra = exynos4412_pmu_config, > +}; > + > +static struct exynos_pmu_data exynos5250_pmu_data = { static const struct > + .pmu_config = exynos5250_pmu_config, > + .pmu_init = exynos5250_pmu_init, > + .powerdown_conf = exynos5_powerdown_conf, > +}; > > - exynos_pmu_config = exynos4210_pmu_config; > - > - if (soc_is_exynos4210()) { > - exynos_pmu_config = exynos4210_pmu_config; > - pr_info("EXYNOS4210 PMU Initialize\n"); > - } else if (soc_is_exynos4212() || soc_is_exynos4412()) { > - exynos_pmu_config = exynos4x12_pmu_config; > - pr_info("EXYNOS4x12 PMU Initialize\n"); > - } else if (soc_is_exynos5250()) { > - /* > - * When SYS_WDTRESET is set, watchdog timer reset request > - * is ignored by power management unit. > - */ > - value = __raw_readl(pmu_base_addr + EXYNOS5_AUTO_WDTRESET_DISABLE); > - value &= ~EXYNOS5_SYS_WDTRESET; > - __raw_writel(value, pmu_base_addr + EXYNOS5_AUTO_WDTRESET_DISABLE); > - > - value = __raw_readl(pmu_base_addr + EXYNOS5_MASK_WDTRESET_REQUEST); > - value &= ~EXYNOS5_SYS_WDTRESET; > - __raw_writel(value, pmu_base_addr + EXYNOS5_MASK_WDTRESET_REQUEST); > - > - exynos_pmu_config = exynos5250_pmu_config; > - pr_info("EXYNOS5250 PMU Initialize\n"); > - } else { > - pr_info("EXYNOS: PMU not supported\n"); > +static struct regmap_config pmu_regmap_config = { static const struct > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > +}; > + > +/* > + * PMU platform driver and devicetree bindings. > + */ > +static struct of_device_id exynos_pmu_of_device_ids[] = { static const struct > + { > + .compatible = "samsung,exynos4210-pmu", > + .data = (void *)&exynos4210_pmu_data, No need for the cast. > + }, > + { nit: You could place both parentheses in the same line, i.e. }, { > + .compatible = "samsung,exynos4212-pmu", > + .data = (void *)&exynos4212_pmu_data, Ditto. > + }, > + { > + .compatible = "samsung,exynos4412-pmu", > + .data = (void *)&exynos4412_pmu_data, Ditto. > + }, > + { > + .compatible = "samsung,exynos5250-pmu", > + .data = (void *)&exynos5250_pmu_data, Ditto. > + }, > + {}, It is a good practice to put a comment inside the sentinel, e.g. { /* sentinel */ } > +}; > + > +static int exynos_pmu_probe(struct platform_device *pdev) > +{ > + const struct of_device_id *match; > + struct device *dev = &pdev->dev; > + struct regmap *regmap; > + struct resource *res; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) > + return -ENOENT; No need to check this here, if you use devm_ioremap_resource() instead. > + > + pmu_base_addr = devm_ioremap(dev, res->start, resource_size(res)); You could use devm_ioremap_resource() instead. In addition to handling the resource directly without the need to access its internals here, it also checks the validity of the resource pointer. > + if (IS_ERR(pmu_base_addr)) > + return PTR_ERR(pmu_base_addr); > + > + pmu_context = devm_kzalloc(&pdev->dev, > + sizeof(struct exynos_pmu_context), > + GFP_KERNEL); > + > + if (pmu_context == NULL) { nit: Extraneous blank line. nit: Inconsistent pointer check (!res above, but == NULL here), just use !ptr everywhere, please. > + dev_err(dev, "Cannot allocate memory.\n"); > + return -ENOMEM; > + } > + > + regmap = devm_regmap_init_mmio(dev, pmu_base_addr, > + &pmu_regmap_config); > + if (IS_ERR(regmap)) { > + dev_err(dev, "regmap init failed\n"); > + return PTR_ERR(regmap); > } > > + devm_syscon_register(dev, regmap); > + > + match = of_match_node(exynos_pmu_of_device_ids, pdev->dev.of_node); > + > + pmu_context->pmu_data = (struct exynos_pmu_data *) match->data; No need to cast if you change all affected pointers to const. > + > + if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init) In what conditions pmu_data will be NULL? > + pmu_context->pmu_data->pmu_init(); > + > + pmu_context->dev = dev; I believe this should be set before calling pmu_init(). > + > + platform_set_drvdata(pdev, pmu_context); > + > + pr_info("Exynos PMU Driver probe done!!!\n"); Hurrah, the driver probed! The users won't care, though, and they might find those exclamation marks scary. ;) dev_dbg() without those exclamation marks would be more appropriate. Best regards, Tomasz
Hi Tomasz, On Monday, June 30, 2014 Tomasz Figa wrote: > Hi Pankaj, > > Please see my comments inline. > > On 25.06.2014 16:03, Pankaj Dubey wrote: > > This patch modifies Exynos Power Management Unit (PMU) initialization > > implementation in following way: > > > > - Added platform driver support and probe function where Exynos PMU > > driver will register itself as syscon provider with syscon framework. > > - Added platform struct exynos_pmu_data to hold platform specific data. > > - For each SoC's PMU support now we can add platform data and statically > > bind PMU configuration and SoC specific initialization function. > > - Separate each SoC's PMU initialization function and make it as part of > > platform data. > > - It also removes uses of soc_is_exynosXYZ(). > > [snip] > > > @@ -93,7 +112,7 @@ static const struct exynos_pmu_conf > exynos4210_pmu_config[] = { > > { PMU_TABLE_END,}, > > }; > > > > -static const struct exynos_pmu_conf exynos4x12_pmu_config[] = { > > +static const struct exynos_pmu_conf exynos4212_pmu_config[] = { > > Why the name change? OK, looks like by mistake I replaced all exynos4x12 with exynos4212, I will correct this. > > > { S5P_ARM_CORE0_LOWPWR, { 0x0, 0x0, 0x2 } }, > > { S5P_DIS_IRQ_CORE0, { 0x0, 0x0, 0x0 } }, > > { S5P_DIS_IRQ_CENTRAL0, { 0x0, 0x0, 0x0 } }, > > @@ -335,7 +354,7 @@ static unsigned int const exynos5_list_diable_wfi_wfe[] = > { > > EXYNOS5_ISP_ARM_OPTION, > > }; > > > > -static void exynos5_init_pmu(void) > > +static void exynos5_powerdown_conf(enum sys_powerdown mode) > > { > > unsigned int i; > > unsigned int tmp; > > @@ -372,51 +391,151 @@ void exynos_sys_powerdown_conf(enum > > sys_powerdown mode) { > > unsigned int i; > > > > - if (soc_is_exynos5250()) > > - exynos5_init_pmu(); > > + struct exynos_pmu_data *pmu_data = pmu_context->pmu_data; > > + > > + if (!pmu_data) > > + return; > > > > - for (i = 0; (exynos_pmu_config[i].offset != PMU_TABLE_END) ; i++) > > - __raw_writel(exynos_pmu_config[i].val[mode], > > - pmu_base_addr + exynos_pmu_config[i].offset); > > + if (pmu_data->powerdown_conf) > > + pmu_data->powerdown_conf(mode); > > > > - if (soc_is_exynos4412()) { > > - for (i = 0; exynos4412_pmu_config[i].offset != PMU_TABLE_END ; > i++) > > - __raw_writel(exynos4412_pmu_config[i].val[mode], > > - pmu_base_addr + exynos4412_pmu_config[i].offset); > > + if (pmu_data->pmu_config) { > > + for (i = 0; (pmu_data->pmu_config[i].offset != PMU_TABLE_END) > ; i++) > > + __raw_writel(pmu_data->pmu_config[i].val[mode], > > + pmu_base_addr + pmu_data- > >pmu_config[i].offset); > > Analogically to patch 2/5, you could add static inline accessors and use them instead > of adding pmu_base_addr every time. > OK. > > + } > > + > > + if (pmu_data->pmu_config_extra) { > > + for (i = 0; pmu_data->pmu_config_extra[i].offset != > PMU_TABLE_END; i++) > > + __raw_writel(pmu_data->pmu_config_extra[i].val[mode], > > + pmu_base_addr + pmu_data- > >pmu_config_extra[i].offset); > > } > > } > > > > -static int __init exynos_pmu_init(void) > > +static void exynos5250_pmu_init(void) > > { > > unsigned int value; > > + /* > > + * When SYS_WDTRESET is set, watchdog timer reset request > > + * is ignored by power management unit. > > + */ > > + value = __raw_readl(pmu_base_addr + > EXYNOS5_AUTO_WDTRESET_DISABLE); > > + value &= ~EXYNOS5_SYS_WDTRESET; > > + __raw_writel(value, pmu_base_addr + > EXYNOS5_AUTO_WDTRESET_DISABLE); > > + > > + value = __raw_readl(pmu_base_addr + > EXYNOS5_MASK_WDTRESET_REQUEST); > > + value &= ~EXYNOS5_SYS_WDTRESET; > > + __raw_writel(value, pmu_base_addr + > EXYNOS5_MASK_WDTRESET_REQUEST); > > +} > > + > > +static struct exynos_pmu_data exynos4210_pmu_data = { > > static const struct > > > + .pmu_config = exynos4210_pmu_config, > > +}; > > + > > +static struct exynos_pmu_data exynos4212_pmu_data = { > > static const struct > > > + .pmu_config = exynos4212_pmu_config, > > +}; > > + > > +static struct exynos_pmu_data exynos4412_pmu_data = { > > static const struct > > > + .pmu_config = exynos4212_pmu_config, > > + .pmu_config_extra = exynos4412_pmu_config, > > +}; > > + > > +static struct exynos_pmu_data exynos5250_pmu_data = { > > static const struct > Ok, will make all of them const. > > + .pmu_config = exynos5250_pmu_config, > > + .pmu_init = exynos5250_pmu_init, > > + .powerdown_conf = exynos5_powerdown_conf, > > +}; > > > > - exynos_pmu_config = exynos4210_pmu_config; > > - > > - if (soc_is_exynos4210()) { > > - exynos_pmu_config = exynos4210_pmu_config; > > - pr_info("EXYNOS4210 PMU Initialize\n"); > > - } else if (soc_is_exynos4212() || soc_is_exynos4412()) { > > - exynos_pmu_config = exynos4x12_pmu_config; > > - pr_info("EXYNOS4x12 PMU Initialize\n"); > > - } else if (soc_is_exynos5250()) { > > - /* > > - * When SYS_WDTRESET is set, watchdog timer reset request > > - * is ignored by power management unit. > > - */ > > - value = __raw_readl(pmu_base_addr + > EXYNOS5_AUTO_WDTRESET_DISABLE); > > - value &= ~EXYNOS5_SYS_WDTRESET; > > - __raw_writel(value, pmu_base_addr + > EXYNOS5_AUTO_WDTRESET_DISABLE); > > - > > - value = __raw_readl(pmu_base_addr + > EXYNOS5_MASK_WDTRESET_REQUEST); > > - value &= ~EXYNOS5_SYS_WDTRESET; > > - __raw_writel(value, pmu_base_addr + > EXYNOS5_MASK_WDTRESET_REQUEST); > > - > > - exynos_pmu_config = exynos5250_pmu_config; > > - pr_info("EXYNOS5250 PMU Initialize\n"); > > - } else { > > - pr_info("EXYNOS: PMU not supported\n"); > > +static struct regmap_config pmu_regmap_config = { > > static const struct > > > + .reg_bits = 32, > > + .val_bits = 32, > > + .reg_stride = 4, > > +}; > > + > > +/* > > + * PMU platform driver and devicetree bindings. > > + */ > > +static struct of_device_id exynos_pmu_of_device_ids[] = { > > static const struct > > > + { > > + .compatible = "samsung,exynos4210-pmu", > > + .data = (void *)&exynos4210_pmu_data, > > No need for the cast. > > > + }, > > + { > > nit: You could place both parentheses in the same line, i.e. > > }, { > > > + .compatible = "samsung,exynos4212-pmu", > > + .data = (void *)&exynos4212_pmu_data, > > Ditto. > > > + }, > > + { > > + .compatible = "samsung,exynos4412-pmu", > > + .data = (void *)&exynos4412_pmu_data, > > Ditto. > > > + }, > > + { > > + .compatible = "samsung,exynos5250-pmu", > > + .data = (void *)&exynos5250_pmu_data, > > Ditto. > Ok, will remove these. > > + }, > > + {}, > > It is a good practice to put a comment inside the sentinel, e.g. > > { /* sentinel */ } > > > +}; > > + > > +static int exynos_pmu_probe(struct platform_device *pdev) { > > + const struct of_device_id *match; > > + struct device *dev = &pdev->dev; > > + struct regmap *regmap; > > + struct resource *res; > > + > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + if (!res) > > + return -ENOENT; > > No need to check this here, if you use devm_ioremap_resource() instead. > > > + > > + pmu_base_addr = devm_ioremap(dev, res->start, resource_size(res)); > > You could use devm_ioremap_resource() instead. In addition to handling the resource > directly without the need to access its internals here, it also checks the validity of the > resource pointer. > > > + if (IS_ERR(pmu_base_addr)) > > + return PTR_ERR(pmu_base_addr); > > + > > + pmu_context = devm_kzalloc(&pdev->dev, > > + sizeof(struct exynos_pmu_context), > > + GFP_KERNEL); > > + > > + if (pmu_context == NULL) { > > nit: Extraneous blank line. > nit: Inconsistent pointer check (!res above, but == NULL here), just use !ptr > everywhere, please. OK. > > > + dev_err(dev, "Cannot allocate memory.\n"); > > + return -ENOMEM; > > + } > > + > > + regmap = devm_regmap_init_mmio(dev, pmu_base_addr, > > + &pmu_regmap_config); > > + if (IS_ERR(regmap)) { > > + dev_err(dev, "regmap init failed\n"); > > + return PTR_ERR(regmap); > > } > > > > + devm_syscon_register(dev, regmap); > > + > > + match = of_match_node(exynos_pmu_of_device_ids, pdev->dev.of_node); > > + > > + pmu_context->pmu_data = (struct exynos_pmu_data *) match->data; > > No need to cast if you change all affected pointers to const. > OK. > > + > > + if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init) > > In what conditions pmu_data will be NULL? > What if we want driver to be probed but no data has been given? If driver gets probed it still can act as syscon provider, and other IPs can access PMU registers, but PMU functionality itself will not be available, for same reason I have put a check for pmu_data in powerdown_conf also. > > + pmu_context->pmu_data->pmu_init(); > > + > > + pmu_context->dev = dev; > > I believe this should be set before calling pmu_init(). > OK. > > + > > + platform_set_drvdata(pdev, pmu_context); > > + > > + pr_info("Exynos PMU Driver probe done!!!\n"); > > Hurrah, the driver probed! The users won't care, though, and they might find those > exclamation marks scary. ;) > > dev_dbg() without those exclamation marks would be more appropriate. > OK. > Best regards, > Tomasz Thanks, Pankaj Dubey
On 05.07.2014 09:09, Pankaj Dubey wrote: > On Monday, June 30, 2014 Tomasz Figa wrote: >> On 25.06.2014 16:03, Pankaj Dubey wrote: >>> + >>> + if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init) >> >> In what conditions pmu_data will be NULL? >> > > What if we want driver to be probed but no data has been given? > If driver gets probed it still can act as syscon provider, and other IPs can > access > PMU registers, but PMU functionality itself will not be available, for same > reason > I have put a check for pmu_data in powerdown_conf also. > Is there any point in probing this driver just to register the syscon? AFAIK if this driver errors out, the syscon platform driver will take over and do what we want. That's why we have the second "syscon" compatible string. Best regards, Tomasz
diff --git a/arch/arm/mach-exynos/Kconfig b/arch/arm/mach-exynos/Kconfig index d58995c9..c4320e6 100644 --- a/arch/arm/mach-exynos/Kconfig +++ b/arch/arm/mach-exynos/Kconfig @@ -25,6 +25,7 @@ config ARCH_EXYNOS select PM_GENERIC_DOMAINS if PM_RUNTIME select S5P_DEV_MFC select SRAM + select MFD_SYSCON help Support for SAMSUNG EXYNOS SoCs (EXYNOS4/5) diff --git a/arch/arm/mach-exynos/pmu.c b/arch/arm/mach-exynos/pmu.c index a6f034c..c80a648 100644 --- a/arch/arm/mach-exynos/pmu.c +++ b/arch/arm/mach-exynos/pmu.c @@ -1,5 +1,5 @@ /* - * Copyright (c) 2011-2012 Samsung Electronics Co., Ltd. + * Copyright (c) 2011-2014 Samsung Electronics Co., Ltd. * http://www.samsung.com/ * * EXYNOS - CPU PMU(Power Management Unit) support @@ -9,13 +9,32 @@ * published by the Free Software Foundation. */ +#include <linux/module.h> #include <linux/io.h> -#include <linux/kernel.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/slab.h> +#include <linux/regmap.h> +#include <linux/mfd/syscon.h> #include "common.h" #include "regs-pmu.h" -static const struct exynos_pmu_conf *exynos_pmu_config; +struct exynos_pmu_data { + const struct exynos_pmu_conf *pmu_config; + const struct exynos_pmu_conf *pmu_config_extra; + + void (*pmu_init)(void); + void (*powerdown_conf)(enum sys_powerdown); +}; + +struct exynos_pmu_context { + struct device *dev; + struct exynos_pmu_data *pmu_data; +}; + +static void __iomem *pmu_base_addr; +static struct exynos_pmu_context *pmu_context; static const struct exynos_pmu_conf exynos4210_pmu_config[] = { /* { .offset = offset, .val = { AFTR, LPA, SLEEP } */ @@ -93,7 +112,7 @@ static const struct exynos_pmu_conf exynos4210_pmu_config[] = { { PMU_TABLE_END,}, }; -static const struct exynos_pmu_conf exynos4x12_pmu_config[] = { +static const struct exynos_pmu_conf exynos4212_pmu_config[] = { { S5P_ARM_CORE0_LOWPWR, { 0x0, 0x0, 0x2 } }, { S5P_DIS_IRQ_CORE0, { 0x0, 0x0, 0x0 } }, { S5P_DIS_IRQ_CENTRAL0, { 0x0, 0x0, 0x0 } }, @@ -335,7 +354,7 @@ static unsigned int const exynos5_list_diable_wfi_wfe[] = { EXYNOS5_ISP_ARM_OPTION, }; -static void exynos5_init_pmu(void) +static void exynos5_powerdown_conf(enum sys_powerdown mode) { unsigned int i; unsigned int tmp; @@ -372,51 +391,151 @@ void exynos_sys_powerdown_conf(enum sys_powerdown mode) { unsigned int i; - if (soc_is_exynos5250()) - exynos5_init_pmu(); + struct exynos_pmu_data *pmu_data = pmu_context->pmu_data; + + if (!pmu_data) + return; - for (i = 0; (exynos_pmu_config[i].offset != PMU_TABLE_END) ; i++) - __raw_writel(exynos_pmu_config[i].val[mode], - pmu_base_addr + exynos_pmu_config[i].offset); + if (pmu_data->powerdown_conf) + pmu_data->powerdown_conf(mode); - if (soc_is_exynos4412()) { - for (i = 0; exynos4412_pmu_config[i].offset != PMU_TABLE_END ; i++) - __raw_writel(exynos4412_pmu_config[i].val[mode], - pmu_base_addr + exynos4412_pmu_config[i].offset); + if (pmu_data->pmu_config) { + for (i = 0; (pmu_data->pmu_config[i].offset != PMU_TABLE_END) ; i++) + __raw_writel(pmu_data->pmu_config[i].val[mode], + pmu_base_addr + pmu_data->pmu_config[i].offset); + } + + if (pmu_data->pmu_config_extra) { + for (i = 0; pmu_data->pmu_config_extra[i].offset != PMU_TABLE_END; i++) + __raw_writel(pmu_data->pmu_config_extra[i].val[mode], + pmu_base_addr + pmu_data->pmu_config_extra[i].offset); } } -static int __init exynos_pmu_init(void) +static void exynos5250_pmu_init(void) { unsigned int value; + /* + * When SYS_WDTRESET is set, watchdog timer reset request + * is ignored by power management unit. + */ + value = __raw_readl(pmu_base_addr + EXYNOS5_AUTO_WDTRESET_DISABLE); + value &= ~EXYNOS5_SYS_WDTRESET; + __raw_writel(value, pmu_base_addr + EXYNOS5_AUTO_WDTRESET_DISABLE); + + value = __raw_readl(pmu_base_addr + EXYNOS5_MASK_WDTRESET_REQUEST); + value &= ~EXYNOS5_SYS_WDTRESET; + __raw_writel(value, pmu_base_addr + EXYNOS5_MASK_WDTRESET_REQUEST); +} + +static struct exynos_pmu_data exynos4210_pmu_data = { + .pmu_config = exynos4210_pmu_config, +}; + +static struct exynos_pmu_data exynos4212_pmu_data = { + .pmu_config = exynos4212_pmu_config, +}; + +static struct exynos_pmu_data exynos4412_pmu_data = { + .pmu_config = exynos4212_pmu_config, + .pmu_config_extra = exynos4412_pmu_config, +}; + +static struct exynos_pmu_data exynos5250_pmu_data = { + .pmu_config = exynos5250_pmu_config, + .pmu_init = exynos5250_pmu_init, + .powerdown_conf = exynos5_powerdown_conf, +}; - exynos_pmu_config = exynos4210_pmu_config; - - if (soc_is_exynos4210()) { - exynos_pmu_config = exynos4210_pmu_config; - pr_info("EXYNOS4210 PMU Initialize\n"); - } else if (soc_is_exynos4212() || soc_is_exynos4412()) { - exynos_pmu_config = exynos4x12_pmu_config; - pr_info("EXYNOS4x12 PMU Initialize\n"); - } else if (soc_is_exynos5250()) { - /* - * When SYS_WDTRESET is set, watchdog timer reset request - * is ignored by power management unit. - */ - value = __raw_readl(pmu_base_addr + EXYNOS5_AUTO_WDTRESET_DISABLE); - value &= ~EXYNOS5_SYS_WDTRESET; - __raw_writel(value, pmu_base_addr + EXYNOS5_AUTO_WDTRESET_DISABLE); - - value = __raw_readl(pmu_base_addr + EXYNOS5_MASK_WDTRESET_REQUEST); - value &= ~EXYNOS5_SYS_WDTRESET; - __raw_writel(value, pmu_base_addr + EXYNOS5_MASK_WDTRESET_REQUEST); - - exynos_pmu_config = exynos5250_pmu_config; - pr_info("EXYNOS5250 PMU Initialize\n"); - } else { - pr_info("EXYNOS: PMU not supported\n"); +static struct regmap_config pmu_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, +}; + +/* + * PMU platform driver and devicetree bindings. + */ +static struct of_device_id exynos_pmu_of_device_ids[] = { + { + .compatible = "samsung,exynos4210-pmu", + .data = (void *)&exynos4210_pmu_data, + }, + { + .compatible = "samsung,exynos4212-pmu", + .data = (void *)&exynos4212_pmu_data, + }, + { + .compatible = "samsung,exynos4412-pmu", + .data = (void *)&exynos4412_pmu_data, + }, + { + .compatible = "samsung,exynos5250-pmu", + .data = (void *)&exynos5250_pmu_data, + }, + {}, +}; + +static int exynos_pmu_probe(struct platform_device *pdev) +{ + const struct of_device_id *match; + struct device *dev = &pdev->dev; + struct regmap *regmap; + struct resource *res; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + if (!res) + return -ENOENT; + + pmu_base_addr = devm_ioremap(dev, res->start, resource_size(res)); + if (IS_ERR(pmu_base_addr)) + return PTR_ERR(pmu_base_addr); + + pmu_context = devm_kzalloc(&pdev->dev, + sizeof(struct exynos_pmu_context), + GFP_KERNEL); + + if (pmu_context == NULL) { + dev_err(dev, "Cannot allocate memory.\n"); + return -ENOMEM; + } + + regmap = devm_regmap_init_mmio(dev, pmu_base_addr, + &pmu_regmap_config); + if (IS_ERR(regmap)) { + dev_err(dev, "regmap init failed\n"); + return PTR_ERR(regmap); } + devm_syscon_register(dev, regmap); + + match = of_match_node(exynos_pmu_of_device_ids, pdev->dev.of_node); + + pmu_context->pmu_data = (struct exynos_pmu_data *) match->data; + + if (pmu_context->pmu_data && pmu_context->pmu_data->pmu_init) + pmu_context->pmu_data->pmu_init(); + + pmu_context->dev = dev; + + platform_set_drvdata(pdev, pmu_context); + + pr_info("Exynos PMU Driver probe done!!!\n"); return 0; } -arch_initcall(exynos_pmu_init); + +static struct platform_driver exynos_pmu_driver = { + .driver = { + .name = "exynos-pmu", + .owner = THIS_MODULE, + .of_match_table = exynos_pmu_of_device_ids, + }, + .probe = exynos_pmu_probe, +}; + +static int __init exynos_pmu_init(void) +{ + return platform_driver_register(&exynos_pmu_driver); + +} +postcore_initcall(exynos_pmu_init);
This patch modifies Exynos Power Management Unit (PMU) initialization implementation in following way: - Added platform driver support and probe function where Exynos PMU driver will register itself as syscon provider with syscon framework. - Added platform struct exynos_pmu_data to hold platform specific data. - For each SoC's PMU support now we can add platform data and statically bind PMU configuration and SoC specific initialization function. - Separate each SoC's PMU initialization function and make it as part of platform data. - It also removes uses of soc_is_exynosXYZ(). Signed-off-by: Pankaj Dubey <pankaj.dubey@samsung.com> --- arch/arm/mach-exynos/Kconfig | 1 + arch/arm/mach-exynos/pmu.c | 201 +++++++++++++++++++++++++++++++++--------- 2 files changed, 161 insertions(+), 41 deletions(-)