* [PATCH 0/7] PM / devfreq: Update the devfreq and devfreq-event device
@ 2016-12-15 9:30 Chanwoo Choi
2016-12-15 9:30 ` [PATCH 1/7] PM / devfreq: exynos-bus: Add the detailed correlation for Exynos5433 Chanwoo Choi
` (6 more replies)
0 siblings, 7 replies; 17+ messages in thread
From: Chanwoo Choi @ 2016-12-15 9:30 UTC (permalink / raw)
To: myungjoo.ham, kyungmin.park
Cc: rjw, chanwoo, linux-pm, linux-kernel, Chanwoo Choi
This patches update the devfreq and devfreq-event device. I add the summary
of each patch as following.
- Patch1 is documented for bus frequency scaling of Exynos5433's VDD_INT.
The related-to patches[1] were merged by Exynos SoC maintainer.
- Patch2 uses the regmap interface instead of raw_read/write function.
- Patch3/4 print the useful log for exynos-bus.c and exynos-ppmu.c driver.
- Patch5 fixes the checkpatch warning of devfreq.c.
- Patch6/7 modify the name of sysfs entry for devfreq/devfreq-event device
[1] https://lkml.org/lkml/2016/12/7/846
- ("[PATCH v2 0/5] arm64: dts: Enable bus frequency scaling on Exynos5433-based TM2 board")
Chanwoo Choi (7):
PM / devfreq: exynos-bus: Add the detailed correlation for Exynos5433
PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers
PM / devfreq: exynos-bus: Print the real clock rate of bus
PM / devfreq: exynos-ppmu: Show the registred device for ppmu device
PM / devfreq: Fix the checkpatch warnings
PM / devfreq: Modify the device name as devfreq[X] for sysfs
PM / devfreq: Simplify the sysfs name of devfreq-event device
.../devicetree/bindings/devfreq/exynos-bus.txt | 14 +
drivers/devfreq/devfreq-event.c | 2 +-
drivers/devfreq/devfreq.c | 17 +-
drivers/devfreq/event/exynos-ppmu.c | 331 +++++++++++++++------
drivers/devfreq/exynos-bus.c | 8 +-
5 files changed, 270 insertions(+), 102 deletions(-)
--
1.9.1
^ permalink raw reply [flat|nested] 17+ messages in thread* [PATCH 1/7] PM / devfreq: exynos-bus: Add the detailed correlation for Exynos5433 2016-12-15 9:30 [PATCH 0/7] PM / devfreq: Update the devfreq and devfreq-event device Chanwoo Choi @ 2016-12-15 9:30 ` Chanwoo Choi 2016-12-15 9:30 ` [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers Chanwoo Choi ` (5 subsequent siblings) 6 siblings, 0 replies; 17+ messages in thread From: Chanwoo Choi @ 2016-12-15 9:30 UTC (permalink / raw) To: myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Chanwoo Choi, Rob Herring, devicetree This patch adds the detailed corrleation between sub-blocks and VDD_INT power line for Exynos5433. VDD_INT provided the power source to INT (Internal) block. Cc: Rob Herring <robh+dt@kernel.org> Cc: devicetree@vger.kernel.org Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> --- Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt index d3ec8e676b6b..d085ef90d27c 100644 --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt @@ -123,6 +123,20 @@ Detailed correlation between sub-blocks and power line according to Exynos SoC: |--- FSYS |--- FSYS2 +- In case of Exynos5433, there is VDD_INT power line as following: + VDD_INT |--- G2D (parent device) + |--- MSCL + |--- GSCL + |--- JPEG + |--- MFC + |--- HEVC + |--- BUS0 + |--- BUS1 + |--- BUS2 + |--- PERIS (Fixed clock rate) + |--- PERIC (Fixed clock rate) + |--- FSYS (Fixed clock rate) + Example1: Show the AXI buses of Exynos3250 SoC. Exynos3250 divides the buses to power line (regulator). The MIF (Memory Interface) AXI bus is used to -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-15 9:30 [PATCH 0/7] PM / devfreq: Update the devfreq and devfreq-event device Chanwoo Choi 2016-12-15 9:30 ` [PATCH 1/7] PM / devfreq: exynos-bus: Add the detailed correlation for Exynos5433 Chanwoo Choi @ 2016-12-15 9:30 ` Chanwoo Choi 2016-12-19 19:47 ` Tobias Jakobi 2016-12-15 9:30 ` [PATCH 3/7] PM / devfreq: exynos-bus: Print the real clock rate of bus Chanwoo Choi ` (4 subsequent siblings) 6 siblings, 1 reply; 17+ messages in thread From: Chanwoo Choi @ 2016-12-15 9:30 UTC (permalink / raw) To: myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Chanwoo Choi, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc This patch uses the regmap interface to read and write the registers for exynos PPMU device instead of the legacy memory map functions. Cc: Kukjin Kim <kgene@kernel.org> Cc: Krzysztof Kozlowski <krzk@kernel.org> Cc: Javier Martinez Canillas <javier@osg.samsung.com> Cc: linux-samsung-soc@vger.kernel.org Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/devfreq/event/exynos-ppmu.c | 326 ++++++++++++++++++++++++++---------- 1 file changed, 237 insertions(+), 89 deletions(-) diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c index 107eb91a9415..fb3706faf5bd 100644 --- a/drivers/devfreq/event/exynos-ppmu.c +++ b/drivers/devfreq/event/exynos-ppmu.c @@ -17,13 +17,13 @@ #include <linux/module.h> #include <linux/of_address.h> #include <linux/platform_device.h> +#include <linux/regmap.h> #include <linux/suspend.h> #include <linux/devfreq-event.h> #include "exynos-ppmu.h" struct exynos_ppmu_data { - void __iomem *base; struct clk *clk; }; @@ -33,6 +33,7 @@ struct exynos_ppmu { unsigned int num_events; struct device *dev; + struct regmap *regmap; struct exynos_ppmu_data ppmu; }; @@ -107,20 +108,28 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev) static int exynos_ppmu_disable(struct devfreq_event_dev *edev) { struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); + int ret; u32 pmnc; /* Disable all counters */ - __raw_writel(PPMU_CCNT_MASK | - PPMU_PMCNT0_MASK | - PPMU_PMCNT1_MASK | - PPMU_PMCNT2_MASK | - PPMU_PMCNT3_MASK, - info->ppmu.base + PPMU_CNTENC); + ret = regmap_write(info->regmap, PPMU_CNTENC, + PPMU_CCNT_MASK | + PPMU_PMCNT0_MASK | + PPMU_PMCNT1_MASK | + PPMU_PMCNT2_MASK | + PPMU_PMCNT3_MASK); + if (ret < 0) + return ret; /* Disable PPMU */ - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); + if (ret < 0) + return ret; + pmnc &= ~PPMU_PMNC_ENABLE_MASK; - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); + if (ret < 0) + return ret; return 0; } @@ -129,29 +138,42 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev) { struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); int id = exynos_ppmu_find_ppmu_id(edev); + int ret; u32 pmnc, cntens; if (id < 0) return id; /* Enable specific counter */ - cntens = __raw_readl(info->ppmu.base + PPMU_CNTENS); + ret = regmap_read(info->regmap, PPMU_CNTENS, &cntens); + if (ret < 0) + return ret; + cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); - __raw_writel(cntens, info->ppmu.base + PPMU_CNTENS); + ret = regmap_write(info->regmap, PPMU_CNTENS, cntens); + if (ret < 0) + return ret; /* Set the event of Read/Write data count */ - __raw_writel(PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT, - info->ppmu.base + PPMU_BEVTxSEL(id)); + ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), + PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT); + if (ret < 0) + return ret; /* Reset cycle counter/performance counter and enable PPMU */ - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); + if (ret < 0) + return ret; + pmnc &= ~(PPMU_PMNC_ENABLE_MASK | PPMU_PMNC_COUNTER_RESET_MASK | PPMU_PMNC_CC_RESET_MASK); pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT); pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); + if (ret < 0) + return ret; return 0; } @@ -161,40 +183,64 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, { struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); int id = exynos_ppmu_find_ppmu_id(edev); - u32 pmnc, cntenc; + unsigned int total_count, load_count; + unsigned int pmcnt3_high, pmcnt3_low; + unsigned int pmnc, cntenc; + int ret; if (id < 0) return -EINVAL; /* Disable PPMU */ - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); + if (ret < 0) + return ret; + pmnc &= ~PPMU_PMNC_ENABLE_MASK; - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); + if (ret < 0) + return ret; /* Read cycle count */ - edata->total_count = __raw_readl(info->ppmu.base + PPMU_CCNT); + ret = regmap_read(info->regmap, PPMU_CCNT, &total_count); + if (ret < 0) + return ret; + edata->total_count = total_count; /* Read performance count */ switch (id) { case PPMU_PMNCNT0: case PPMU_PMNCNT1: case PPMU_PMNCNT2: - edata->load_count - = __raw_readl(info->ppmu.base + PPMU_PMNCT(id)); + ret = regmap_read(info->regmap, PPMU_PMNCT(id), &load_count); + if (ret < 0) + return ret; + edata->load_count = load_count; break; case PPMU_PMNCNT3: - edata->load_count = - ((__raw_readl(info->ppmu.base + PPMU_PMCNT3_HIGH) << 8) - | __raw_readl(info->ppmu.base + PPMU_PMCNT3_LOW)); + ret = regmap_read(info->regmap, PPMU_PMCNT3_HIGH, &pmcnt3_high); + if (ret < 0) + return ret; + + ret = regmap_read(info->regmap, PPMU_PMCNT3_LOW, &pmcnt3_low); + if (ret < 0) + return ret; + + edata->load_count = ((pmcnt3_high << 8) | pmcnt3_low); break; default: return -EINVAL; } /* Disable specific counter */ - cntenc = __raw_readl(info->ppmu.base + PPMU_CNTENC); + ret = regmap_read(info->regmap, PPMU_CNTENC, &cntenc); + if (ret < 0) + return ret; + cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); - __raw_writel(cntenc, info->ppmu.base + PPMU_CNTENC); + ret = regmap_write(info->regmap, PPMU_CNTENC, cntenc); + if (ret < 0) + return ret; dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name, edata->load_count, edata->total_count); @@ -214,36 +260,93 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) { struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); + int ret; u32 pmnc, clear; /* Disable all counters */ clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK | PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK); + ret = regmap_write(info->regmap, PPMU_V2_FLAG, clear); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_INTENC, clear); + if (ret < 0) + return ret; - __raw_writel(clear, info->ppmu.base + PPMU_V2_FLAG); - __raw_writel(clear, info->ppmu.base + PPMU_V2_INTENC); - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNTENC); - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNT_RESET); - - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG0); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG1); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG2); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_RESULT); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CNT_AUTO); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV0_TYPE); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV1_TYPE); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV2_TYPE); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV3_TYPE); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_V); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_A); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_V); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_A); - __raw_writel(0x0, info->ppmu.base + PPMU_V2_INTERRUPT_RESET); + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, clear); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CNT_RESET, clear); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG0, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG1, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG2, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CIG_RESULT, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CNT_AUTO, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CH_EV0_TYPE, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CH_EV1_TYPE, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CH_EV2_TYPE, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_CH_EV3_TYPE, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_V, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_A, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_V, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_A, 0x0); + if (ret < 0) + return ret; + + ret = regmap_write(info->regmap, PPMU_V2_INTERRUPT_RESET, 0x0); + if (ret < 0) + return ret; /* Disable PPMU */ - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); + if (ret < 0) + return ret; + pmnc &= ~PPMU_PMNC_ENABLE_MASK; - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); + if (ret < 0) + return ret; return 0; } @@ -251,30 +354,43 @@ static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) { struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); + unsigned int pmnc, cntens; int id = exynos_ppmu_find_ppmu_id(edev); - u32 pmnc, cntens; + int ret; /* Enable all counters */ - cntens = __raw_readl(info->ppmu.base + PPMU_V2_CNTENS); + ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens); + if (ret < 0) + return ret; + cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); - __raw_writel(cntens, info->ppmu.base + PPMU_V2_CNTENS); + ret = regmap_write(info->regmap, PPMU_V2_CNTENS, cntens); + if (ret < 0) + return ret; /* Set the event of Read/Write data count */ switch (id) { case PPMU_PMNCNT0: case PPMU_PMNCNT1: case PPMU_PMNCNT2: - __raw_writel(PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT, - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), + PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT); + if (ret < 0) + return ret; break; case PPMU_PMNCNT3: - __raw_writel(PPMU_V2_EVT3_RW_DATA_CNT, - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), + PPMU_V2_EVT3_RW_DATA_CNT); + if (ret < 0) + return ret; break; } /* Reset cycle counter/performance counter and enable PPMU */ - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); + if (ret < 0) + return ret; + pmnc &= ~(PPMU_PMNC_ENABLE_MASK | PPMU_PMNC_COUNTER_RESET_MASK | PPMU_PMNC_CC_RESET_MASK @@ -284,7 +400,10 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); pmnc |= (PPMU_V2_MODE_MANUAL << PPMU_V2_PMNC_START_MODE_SHIFT); - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); + + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); + if (ret < 0) + return ret; return 0; } @@ -294,37 +413,61 @@ static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev, { struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); int id = exynos_ppmu_find_ppmu_id(edev); - u32 pmnc, cntenc; - u32 pmcnt_high, pmcnt_low; - u64 load_count = 0; + int ret; + unsigned int pmnc, cntenc; + unsigned int pmcnt_high, pmcnt_low; + unsigned int total_count, count; + unsigned long load_count = 0; /* Disable PPMU */ - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); + if (ret < 0) + return ret; + pmnc &= ~PPMU_PMNC_ENABLE_MASK; - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); + if (ret < 0) + return ret; /* Read cycle count and performance count */ - edata->total_count = __raw_readl(info->ppmu.base + PPMU_V2_CCNT); + ret = regmap_read(info->regmap, PPMU_V2_CCNT, &total_count); + if (ret < 0) + return ret; + edata->total_count = total_count; switch (id) { case PPMU_PMNCNT0: case PPMU_PMNCNT1: case PPMU_PMNCNT2: - load_count = __raw_readl(info->ppmu.base + PPMU_V2_PMNCT(id)); + ret = regmap_read(info->regmap, PPMU_V2_PMNCT(id), &count); + if (ret < 0) + return ret; + load_count = count; break; case PPMU_PMNCNT3: - pmcnt_high = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_HIGH); - pmcnt_low = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_LOW); - load_count = ((u64)((pmcnt_high & 0xff)) << 32) - + (u64)pmcnt_low; + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_HIGH, + &pmcnt_high); + if (ret < 0) + return ret; + + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_LOW, &pmcnt_low); + if (ret < 0) + return ret; + + load_count = ((u64)((pmcnt_high & 0xff)) << 32)+ (u64)pmcnt_low; break; } edata->load_count = load_count; /* Disable all counters */ - cntenc = __raw_readl(info->ppmu.base + PPMU_V2_CNTENC); + ret = regmap_read(info->regmap, PPMU_V2_CNTENC, &cntenc); + if (ret < 0) + return 0; + cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); - __raw_writel(cntenc, info->ppmu.base + PPMU_V2_CNTENC); + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, cntenc); + if (ret < 0) + return ret; dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name, edata->load_count, edata->total_count); @@ -411,10 +554,19 @@ static int of_get_devfreq_events(struct device_node *np, return 0; } -static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) +static struct regmap_config exynos_ppmu_regmap_config = { + .reg_bits = 32, + .val_bits = 32, + .reg_stride = 4, +}; + +static int exynos_ppmu_parse_dt(struct platform_device *pdev, + struct exynos_ppmu *info) { struct device *dev = info->dev; struct device_node *np = dev->of_node; + struct resource *res; + void __iomem *base; int ret = 0; if (!np) { @@ -423,10 +575,17 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) } /* Maps the memory mapped IO to control PPMU register */ - info->ppmu.base = of_iomap(np, 0); - if (IS_ERR_OR_NULL(info->ppmu.base)) { - dev_err(dev, "failed to map memory region\n"); - return -ENOMEM; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + base = devm_ioremap_resource(dev, res); + if (IS_ERR(base)) + return PTR_ERR(base); + + exynos_ppmu_regmap_config.max_register = resource_size(res) - 4; + info->regmap = devm_regmap_init_mmio(dev, base, + &exynos_ppmu_regmap_config); + if (IS_ERR(info->regmap)) { + dev_err(dev, "failed to initialize regmap\n"); + return PTR_ERR(info->regmap); } info->ppmu.clk = devm_clk_get(dev, "ppmu"); @@ -438,15 +597,10 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) ret = of_get_devfreq_events(np, info); if (ret < 0) { dev_err(dev, "failed to parse exynos ppmu dt node\n"); - goto err; + return ret; } return 0; - -err: - iounmap(info->ppmu.base); - - return ret; } static int exynos_ppmu_probe(struct platform_device *pdev) @@ -463,7 +617,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) info->dev = &pdev->dev; /* Parse dt data to get resource */ - ret = exynos_ppmu_parse_dt(info); + ret = exynos_ppmu_parse_dt(pdev, info); if (ret < 0) { dev_err(&pdev->dev, "failed to parse devicetree for resource\n"); @@ -476,8 +630,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) if (!info->edev) { dev_err(&pdev->dev, "failed to allocate memory devfreq-event devices\n"); - ret = -ENOMEM; - goto err; + return -ENOMEM; } edev = info->edev; platform_set_drvdata(pdev, info); @@ -488,17 +641,13 @@ static int exynos_ppmu_probe(struct platform_device *pdev) ret = PTR_ERR(edev[i]); dev_err(&pdev->dev, "failed to add devfreq-event device\n"); - goto err; + return PTR_ERR(edev[i]); } } clk_prepare_enable(info->ppmu.clk); return 0; -err: - iounmap(info->ppmu.base); - - return ret; } static int exynos_ppmu_remove(struct platform_device *pdev) @@ -506,7 +655,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev) struct exynos_ppmu *info = platform_get_drvdata(pdev); clk_disable_unprepare(info->ppmu.clk); - iounmap(info->ppmu.base); return 0; } -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-15 9:30 ` [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers Chanwoo Choi @ 2016-12-19 19:47 ` Tobias Jakobi 2016-12-20 2:35 ` Chanwoo Choi 0 siblings, 1 reply; 17+ messages in thread From: Tobias Jakobi @ 2016-12-19 19:47 UTC (permalink / raw) To: Chanwoo Choi, myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc Hello, I was just wondering what is improved by moving to regmap. For me this looks like it only complicates the code. Lots of regmap_{read,write}() and for each one of these we need to check the return code. Also when exactly did __raw_writel() and friends become legacy? With best wishes, Tobias Chanwoo Choi wrote: > This patch uses the regmap interface to read and write the registers for exynos > PPMU device instead of the legacy memory map functions. > > Cc: Kukjin Kim <kgene@kernel.org> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Javier Martinez Canillas <javier@osg.samsung.com> > Cc: linux-samsung-soc@vger.kernel.org > Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> > --- > drivers/devfreq/event/exynos-ppmu.c | 326 ++++++++++++++++++++++++++---------- > 1 file changed, 237 insertions(+), 89 deletions(-) > > diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c > index 107eb91a9415..fb3706faf5bd 100644 > --- a/drivers/devfreq/event/exynos-ppmu.c > +++ b/drivers/devfreq/event/exynos-ppmu.c > @@ -17,13 +17,13 @@ > #include <linux/module.h> > #include <linux/of_address.h> > #include <linux/platform_device.h> > +#include <linux/regmap.h> > #include <linux/suspend.h> > #include <linux/devfreq-event.h> > > #include "exynos-ppmu.h" > > struct exynos_ppmu_data { > - void __iomem *base; > struct clk *clk; > }; > > @@ -33,6 +33,7 @@ struct exynos_ppmu { > unsigned int num_events; > > struct device *dev; > + struct regmap *regmap; > > struct exynos_ppmu_data ppmu; > }; > @@ -107,20 +108,28 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev) > static int exynos_ppmu_disable(struct devfreq_event_dev *edev) > { > struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); > + int ret; > u32 pmnc; > > /* Disable all counters */ > - __raw_writel(PPMU_CCNT_MASK | > - PPMU_PMCNT0_MASK | > - PPMU_PMCNT1_MASK | > - PPMU_PMCNT2_MASK | > - PPMU_PMCNT3_MASK, > - info->ppmu.base + PPMU_CNTENC); > + ret = regmap_write(info->regmap, PPMU_CNTENC, > + PPMU_CCNT_MASK | > + PPMU_PMCNT0_MASK | > + PPMU_PMCNT1_MASK | > + PPMU_PMCNT2_MASK | > + PPMU_PMCNT3_MASK); > + if (ret < 0) > + return ret; > > /* Disable PPMU */ > - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); > + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); > + if (ret < 0) > + return ret; > + > pmnc &= ~PPMU_PMNC_ENABLE_MASK; > - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); > + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); > + if (ret < 0) > + return ret; > > return 0; > } > @@ -129,29 +138,42 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev) > { > struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); > int id = exynos_ppmu_find_ppmu_id(edev); > + int ret; > u32 pmnc, cntens; > > if (id < 0) > return id; > > /* Enable specific counter */ > - cntens = __raw_readl(info->ppmu.base + PPMU_CNTENS); > + ret = regmap_read(info->regmap, PPMU_CNTENS, &cntens); > + if (ret < 0) > + return ret; > + > cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); > - __raw_writel(cntens, info->ppmu.base + PPMU_CNTENS); > + ret = regmap_write(info->regmap, PPMU_CNTENS, cntens); > + if (ret < 0) > + return ret; > > /* Set the event of Read/Write data count */ > - __raw_writel(PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT, > - info->ppmu.base + PPMU_BEVTxSEL(id)); > + ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), > + PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT); > + if (ret < 0) > + return ret; > > /* Reset cycle counter/performance counter and enable PPMU */ > - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); > + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); > + if (ret < 0) > + return ret; > + > pmnc &= ~(PPMU_PMNC_ENABLE_MASK > | PPMU_PMNC_COUNTER_RESET_MASK > | PPMU_PMNC_CC_RESET_MASK); > pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT); > pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); > pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); > - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); > + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); > + if (ret < 0) > + return ret; > > return 0; > } > @@ -161,40 +183,64 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, > { > struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); > int id = exynos_ppmu_find_ppmu_id(edev); > - u32 pmnc, cntenc; > + unsigned int total_count, load_count; > + unsigned int pmcnt3_high, pmcnt3_low; > + unsigned int pmnc, cntenc; > + int ret; > > if (id < 0) > return -EINVAL; > > /* Disable PPMU */ > - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); > + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); > + if (ret < 0) > + return ret; > + > pmnc &= ~PPMU_PMNC_ENABLE_MASK; > - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); > + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); > + if (ret < 0) > + return ret; > > /* Read cycle count */ > - edata->total_count = __raw_readl(info->ppmu.base + PPMU_CCNT); > + ret = regmap_read(info->regmap, PPMU_CCNT, &total_count); > + if (ret < 0) > + return ret; > + edata->total_count = total_count; > > /* Read performance count */ > switch (id) { > case PPMU_PMNCNT0: > case PPMU_PMNCNT1: > case PPMU_PMNCNT2: > - edata->load_count > - = __raw_readl(info->ppmu.base + PPMU_PMNCT(id)); > + ret = regmap_read(info->regmap, PPMU_PMNCT(id), &load_count); > + if (ret < 0) > + return ret; > + edata->load_count = load_count; > break; > case PPMU_PMNCNT3: > - edata->load_count = > - ((__raw_readl(info->ppmu.base + PPMU_PMCNT3_HIGH) << 8) > - | __raw_readl(info->ppmu.base + PPMU_PMCNT3_LOW)); > + ret = regmap_read(info->regmap, PPMU_PMCNT3_HIGH, &pmcnt3_high); > + if (ret < 0) > + return ret; > + > + ret = regmap_read(info->regmap, PPMU_PMCNT3_LOW, &pmcnt3_low); > + if (ret < 0) > + return ret; > + > + edata->load_count = ((pmcnt3_high << 8) | pmcnt3_low); > break; > default: > return -EINVAL; > } > > /* Disable specific counter */ > - cntenc = __raw_readl(info->ppmu.base + PPMU_CNTENC); > + ret = regmap_read(info->regmap, PPMU_CNTENC, &cntenc); > + if (ret < 0) > + return ret; > + > cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); > - __raw_writel(cntenc, info->ppmu.base + PPMU_CNTENC); > + ret = regmap_write(info->regmap, PPMU_CNTENC, cntenc); > + if (ret < 0) > + return ret; > > dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name, > edata->load_count, edata->total_count); > @@ -214,36 +260,93 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, > static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) > { > struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); > + int ret; > u32 pmnc, clear; > > /* Disable all counters */ > clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK > | PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK); > + ret = regmap_write(info->regmap, PPMU_V2_FLAG, clear); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_INTENC, clear); > + if (ret < 0) > + return ret; > > - __raw_writel(clear, info->ppmu.base + PPMU_V2_FLAG); > - __raw_writel(clear, info->ppmu.base + PPMU_V2_INTENC); > - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNTENC); > - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNT_RESET); > - > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG0); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG1); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG2); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_RESULT); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CNT_AUTO); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV0_TYPE); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV1_TYPE); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV2_TYPE); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV3_TYPE); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_V); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_A); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_V); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_A); > - __raw_writel(0x0, info->ppmu.base + PPMU_V2_INTERRUPT_RESET); > + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, clear); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CNT_RESET, clear); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG0, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG1, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG2, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CIG_RESULT, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CNT_AUTO, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CH_EV0_TYPE, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CH_EV1_TYPE, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CH_EV2_TYPE, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_CH_EV3_TYPE, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_V, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_A, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_V, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_A, 0x0); > + if (ret < 0) > + return ret; > + > + ret = regmap_write(info->regmap, PPMU_V2_INTERRUPT_RESET, 0x0); > + if (ret < 0) > + return ret; > > /* Disable PPMU */ > - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); > + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); > + if (ret < 0) > + return ret; > + > pmnc &= ~PPMU_PMNC_ENABLE_MASK; > - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); > + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); > + if (ret < 0) > + return ret; > > return 0; > } > @@ -251,30 +354,43 @@ static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) > static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) > { > struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); > + unsigned int pmnc, cntens; > int id = exynos_ppmu_find_ppmu_id(edev); > - u32 pmnc, cntens; > + int ret; > > /* Enable all counters */ > - cntens = __raw_readl(info->ppmu.base + PPMU_V2_CNTENS); > + ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens); > + if (ret < 0) > + return ret; > + > cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); > - __raw_writel(cntens, info->ppmu.base + PPMU_V2_CNTENS); > + ret = regmap_write(info->regmap, PPMU_V2_CNTENS, cntens); > + if (ret < 0) > + return ret; > > /* Set the event of Read/Write data count */ > switch (id) { > case PPMU_PMNCNT0: > case PPMU_PMNCNT1: > case PPMU_PMNCNT2: > - __raw_writel(PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT, > - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); > + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), > + PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT); > + if (ret < 0) > + return ret; > break; > case PPMU_PMNCNT3: > - __raw_writel(PPMU_V2_EVT3_RW_DATA_CNT, > - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); > + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), > + PPMU_V2_EVT3_RW_DATA_CNT); > + if (ret < 0) > + return ret; > break; > } > > /* Reset cycle counter/performance counter and enable PPMU */ > - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); > + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); > + if (ret < 0) > + return ret; > + > pmnc &= ~(PPMU_PMNC_ENABLE_MASK > | PPMU_PMNC_COUNTER_RESET_MASK > | PPMU_PMNC_CC_RESET_MASK > @@ -284,7 +400,10 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) > pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); > pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); > pmnc |= (PPMU_V2_MODE_MANUAL << PPMU_V2_PMNC_START_MODE_SHIFT); > - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); > + > + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); > + if (ret < 0) > + return ret; > > return 0; > } > @@ -294,37 +413,61 @@ static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev, > { > struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); > int id = exynos_ppmu_find_ppmu_id(edev); > - u32 pmnc, cntenc; > - u32 pmcnt_high, pmcnt_low; > - u64 load_count = 0; > + int ret; > + unsigned int pmnc, cntenc; > + unsigned int pmcnt_high, pmcnt_low; > + unsigned int total_count, count; > + unsigned long load_count = 0; > > /* Disable PPMU */ > - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); > + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); > + if (ret < 0) > + return ret; > + > pmnc &= ~PPMU_PMNC_ENABLE_MASK; > - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); > + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); > + if (ret < 0) > + return ret; > > /* Read cycle count and performance count */ > - edata->total_count = __raw_readl(info->ppmu.base + PPMU_V2_CCNT); > + ret = regmap_read(info->regmap, PPMU_V2_CCNT, &total_count); > + if (ret < 0) > + return ret; > + edata->total_count = total_count; > > switch (id) { > case PPMU_PMNCNT0: > case PPMU_PMNCNT1: > case PPMU_PMNCNT2: > - load_count = __raw_readl(info->ppmu.base + PPMU_V2_PMNCT(id)); > + ret = regmap_read(info->regmap, PPMU_V2_PMNCT(id), &count); > + if (ret < 0) > + return ret; > + load_count = count; > break; > case PPMU_PMNCNT3: > - pmcnt_high = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_HIGH); > - pmcnt_low = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_LOW); > - load_count = ((u64)((pmcnt_high & 0xff)) << 32) > - + (u64)pmcnt_low; > + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_HIGH, > + &pmcnt_high); > + if (ret < 0) > + return ret; > + > + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_LOW, &pmcnt_low); > + if (ret < 0) > + return ret; > + > + load_count = ((u64)((pmcnt_high & 0xff)) << 32)+ (u64)pmcnt_low; > break; > } > edata->load_count = load_count; > > /* Disable all counters */ > - cntenc = __raw_readl(info->ppmu.base + PPMU_V2_CNTENC); > + ret = regmap_read(info->regmap, PPMU_V2_CNTENC, &cntenc); > + if (ret < 0) > + return 0; > + > cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); > - __raw_writel(cntenc, info->ppmu.base + PPMU_V2_CNTENC); > + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, cntenc); > + if (ret < 0) > + return ret; > > dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name, > edata->load_count, edata->total_count); > @@ -411,10 +554,19 @@ static int of_get_devfreq_events(struct device_node *np, > return 0; > } > > -static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) > +static struct regmap_config exynos_ppmu_regmap_config = { > + .reg_bits = 32, > + .val_bits = 32, > + .reg_stride = 4, > +}; > + > +static int exynos_ppmu_parse_dt(struct platform_device *pdev, > + struct exynos_ppmu *info) > { > struct device *dev = info->dev; > struct device_node *np = dev->of_node; > + struct resource *res; > + void __iomem *base; > int ret = 0; > > if (!np) { > @@ -423,10 +575,17 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) > } > > /* Maps the memory mapped IO to control PPMU register */ > - info->ppmu.base = of_iomap(np, 0); > - if (IS_ERR_OR_NULL(info->ppmu.base)) { > - dev_err(dev, "failed to map memory region\n"); > - return -ENOMEM; > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + exynos_ppmu_regmap_config.max_register = resource_size(res) - 4; > + info->regmap = devm_regmap_init_mmio(dev, base, > + &exynos_ppmu_regmap_config); > + if (IS_ERR(info->regmap)) { > + dev_err(dev, "failed to initialize regmap\n"); > + return PTR_ERR(info->regmap); > } > > info->ppmu.clk = devm_clk_get(dev, "ppmu"); > @@ -438,15 +597,10 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) > ret = of_get_devfreq_events(np, info); > if (ret < 0) { > dev_err(dev, "failed to parse exynos ppmu dt node\n"); > - goto err; > + return ret; > } > > return 0; > - > -err: > - iounmap(info->ppmu.base); > - > - return ret; > } > > static int exynos_ppmu_probe(struct platform_device *pdev) > @@ -463,7 +617,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) > info->dev = &pdev->dev; > > /* Parse dt data to get resource */ > - ret = exynos_ppmu_parse_dt(info); > + ret = exynos_ppmu_parse_dt(pdev, info); > if (ret < 0) { > dev_err(&pdev->dev, > "failed to parse devicetree for resource\n"); > @@ -476,8 +630,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) > if (!info->edev) { > dev_err(&pdev->dev, > "failed to allocate memory devfreq-event devices\n"); > - ret = -ENOMEM; > - goto err; > + return -ENOMEM; > } > edev = info->edev; > platform_set_drvdata(pdev, info); > @@ -488,17 +641,13 @@ static int exynos_ppmu_probe(struct platform_device *pdev) > ret = PTR_ERR(edev[i]); > dev_err(&pdev->dev, > "failed to add devfreq-event device\n"); > - goto err; > + return PTR_ERR(edev[i]); > } > } > > clk_prepare_enable(info->ppmu.clk); > > return 0; > -err: > - iounmap(info->ppmu.base); > - > - return ret; > } > > static int exynos_ppmu_remove(struct platform_device *pdev) > @@ -506,7 +655,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev) > struct exynos_ppmu *info = platform_get_drvdata(pdev); > > clk_disable_unprepare(info->ppmu.clk); > - iounmap(info->ppmu.base); > > return 0; > } > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-19 19:47 ` Tobias Jakobi @ 2016-12-20 2:35 ` Chanwoo Choi 2016-12-20 8:08 ` Tobias Jakobi 0 siblings, 1 reply; 17+ messages in thread From: Chanwoo Choi @ 2016-12-20 2:35 UTC (permalink / raw) To: Tobias Jakobi, myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc Hi, On 2016년 12월 20일 04:47, Tobias Jakobi wrote: > Hello, > > I was just wondering what is improved by moving to regmap. For me this > looks like it only complicates the code. Lots of regmap_{read,write}() > and for each one of these we need to check the return code. It is correct to check the return value. It cover all of exception. > > Also when exactly did __raw_writel() and friends become legacy? Also, I want to use the regmap interface (regmap-io) instead of __raw_readl/__raw_writel using the base address directly. Regards, Chanwoo Choi > > With best wishes, > Tobias > > > Chanwoo Choi wrote: >> This patch uses the regmap interface to read and write the registers for exynos >> PPMU device instead of the legacy memory map functions. >> >> Cc: Kukjin Kim <kgene@kernel.org> >> Cc: Krzysztof Kozlowski <krzk@kernel.org> >> Cc: Javier Martinez Canillas <javier@osg.samsung.com> >> Cc: linux-samsung-soc@vger.kernel.org >> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >> --- >> drivers/devfreq/event/exynos-ppmu.c | 326 ++++++++++++++++++++++++++---------- >> 1 file changed, 237 insertions(+), 89 deletions(-) >> >> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c >> index 107eb91a9415..fb3706faf5bd 100644 >> --- a/drivers/devfreq/event/exynos-ppmu.c >> +++ b/drivers/devfreq/event/exynos-ppmu.c >> @@ -17,13 +17,13 @@ >> #include <linux/module.h> >> #include <linux/of_address.h> >> #include <linux/platform_device.h> >> +#include <linux/regmap.h> >> #include <linux/suspend.h> >> #include <linux/devfreq-event.h> >> >> #include "exynos-ppmu.h" >> >> struct exynos_ppmu_data { >> - void __iomem *base; >> struct clk *clk; >> }; >> >> @@ -33,6 +33,7 @@ struct exynos_ppmu { >> unsigned int num_events; >> >> struct device *dev; >> + struct regmap *regmap; >> >> struct exynos_ppmu_data ppmu; >> }; >> @@ -107,20 +108,28 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev) >> static int exynos_ppmu_disable(struct devfreq_event_dev *edev) >> { >> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >> + int ret; >> u32 pmnc; >> >> /* Disable all counters */ >> - __raw_writel(PPMU_CCNT_MASK | >> - PPMU_PMCNT0_MASK | >> - PPMU_PMCNT1_MASK | >> - PPMU_PMCNT2_MASK | >> - PPMU_PMCNT3_MASK, >> - info->ppmu.base + PPMU_CNTENC); >> + ret = regmap_write(info->regmap, PPMU_CNTENC, >> + PPMU_CCNT_MASK | >> + PPMU_PMCNT0_MASK | >> + PPMU_PMCNT1_MASK | >> + PPMU_PMCNT2_MASK | >> + PPMU_PMCNT3_MASK); >> + if (ret < 0) >> + return ret; >> >> /* Disable PPMU */ >> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >> + if (ret < 0) >> + return ret; >> + >> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >> + if (ret < 0) >> + return ret; >> >> return 0; >> } >> @@ -129,29 +138,42 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev) >> { >> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >> int id = exynos_ppmu_find_ppmu_id(edev); >> + int ret; >> u32 pmnc, cntens; >> >> if (id < 0) >> return id; >> >> /* Enable specific counter */ >> - cntens = __raw_readl(info->ppmu.base + PPMU_CNTENS); >> + ret = regmap_read(info->regmap, PPMU_CNTENS, &cntens); >> + if (ret < 0) >> + return ret; >> + >> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >> - __raw_writel(cntens, info->ppmu.base + PPMU_CNTENS); >> + ret = regmap_write(info->regmap, PPMU_CNTENS, cntens); >> + if (ret < 0) >> + return ret; >> >> /* Set the event of Read/Write data count */ >> - __raw_writel(PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT, >> - info->ppmu.base + PPMU_BEVTxSEL(id)); >> + ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), >> + PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT); >> + if (ret < 0) >> + return ret; >> >> /* Reset cycle counter/performance counter and enable PPMU */ >> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >> + if (ret < 0) >> + return ret; >> + >> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >> | PPMU_PMNC_COUNTER_RESET_MASK >> | PPMU_PMNC_CC_RESET_MASK); >> pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT); >> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >> + if (ret < 0) >> + return ret; >> >> return 0; >> } >> @@ -161,40 +183,64 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >> { >> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >> int id = exynos_ppmu_find_ppmu_id(edev); >> - u32 pmnc, cntenc; >> + unsigned int total_count, load_count; >> + unsigned int pmcnt3_high, pmcnt3_low; >> + unsigned int pmnc, cntenc; >> + int ret; >> >> if (id < 0) >> return -EINVAL; >> >> /* Disable PPMU */ >> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >> + if (ret < 0) >> + return ret; >> + >> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >> + if (ret < 0) >> + return ret; >> >> /* Read cycle count */ >> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_CCNT); >> + ret = regmap_read(info->regmap, PPMU_CCNT, &total_count); >> + if (ret < 0) >> + return ret; >> + edata->total_count = total_count; >> >> /* Read performance count */ >> switch (id) { >> case PPMU_PMNCNT0: >> case PPMU_PMNCNT1: >> case PPMU_PMNCNT2: >> - edata->load_count >> - = __raw_readl(info->ppmu.base + PPMU_PMNCT(id)); >> + ret = regmap_read(info->regmap, PPMU_PMNCT(id), &load_count); >> + if (ret < 0) >> + return ret; >> + edata->load_count = load_count; >> break; >> case PPMU_PMNCNT3: >> - edata->load_count = >> - ((__raw_readl(info->ppmu.base + PPMU_PMCNT3_HIGH) << 8) >> - | __raw_readl(info->ppmu.base + PPMU_PMCNT3_LOW)); >> + ret = regmap_read(info->regmap, PPMU_PMCNT3_HIGH, &pmcnt3_high); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_read(info->regmap, PPMU_PMCNT3_LOW, &pmcnt3_low); >> + if (ret < 0) >> + return ret; >> + >> + edata->load_count = ((pmcnt3_high << 8) | pmcnt3_low); >> break; >> default: >> return -EINVAL; >> } >> >> /* Disable specific counter */ >> - cntenc = __raw_readl(info->ppmu.base + PPMU_CNTENC); >> + ret = regmap_read(info->regmap, PPMU_CNTENC, &cntenc); >> + if (ret < 0) >> + return ret; >> + >> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >> - __raw_writel(cntenc, info->ppmu.base + PPMU_CNTENC); >> + ret = regmap_write(info->regmap, PPMU_CNTENC, cntenc); >> + if (ret < 0) >> + return ret; >> >> dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name, >> edata->load_count, edata->total_count); >> @@ -214,36 +260,93 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >> static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >> { >> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >> + int ret; >> u32 pmnc, clear; >> >> /* Disable all counters */ >> clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK >> | PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK); >> + ret = regmap_write(info->regmap, PPMU_V2_FLAG, clear); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_INTENC, clear); >> + if (ret < 0) >> + return ret; >> >> - __raw_writel(clear, info->ppmu.base + PPMU_V2_FLAG); >> - __raw_writel(clear, info->ppmu.base + PPMU_V2_INTENC); >> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNTENC); >> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNT_RESET); >> - >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG0); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG1); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG2); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_RESULT); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CNT_AUTO); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV0_TYPE); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV1_TYPE); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV2_TYPE); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV3_TYPE); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_V); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_A); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_V); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_A); >> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_INTERRUPT_RESET); >> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, clear); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CNT_RESET, clear); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG0, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG1, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG2, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CIG_RESULT, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CNT_AUTO, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV0_TYPE, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV1_TYPE, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV2_TYPE, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV3_TYPE, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_V, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_A, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_V, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_A, 0x0); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_write(info->regmap, PPMU_V2_INTERRUPT_RESET, 0x0); >> + if (ret < 0) >> + return ret; >> >> /* Disable PPMU */ >> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >> + if (ret < 0) >> + return ret; >> + >> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >> + if (ret < 0) >> + return ret; >> >> return 0; >> } >> @@ -251,30 +354,43 @@ static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >> static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >> { >> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >> + unsigned int pmnc, cntens; >> int id = exynos_ppmu_find_ppmu_id(edev); >> - u32 pmnc, cntens; >> + int ret; >> >> /* Enable all counters */ >> - cntens = __raw_readl(info->ppmu.base + PPMU_V2_CNTENS); >> + ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens); >> + if (ret < 0) >> + return ret; >> + >> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >> - __raw_writel(cntens, info->ppmu.base + PPMU_V2_CNTENS); >> + ret = regmap_write(info->regmap, PPMU_V2_CNTENS, cntens); >> + if (ret < 0) >> + return ret; >> >> /* Set the event of Read/Write data count */ >> switch (id) { >> case PPMU_PMNCNT0: >> case PPMU_PMNCNT1: >> case PPMU_PMNCNT2: >> - __raw_writel(PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT, >> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >> + PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT); >> + if (ret < 0) >> + return ret; >> break; >> case PPMU_PMNCNT3: >> - __raw_writel(PPMU_V2_EVT3_RW_DATA_CNT, >> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >> + PPMU_V2_EVT3_RW_DATA_CNT); >> + if (ret < 0) >> + return ret; >> break; >> } >> >> /* Reset cycle counter/performance counter and enable PPMU */ >> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >> + if (ret < 0) >> + return ret; >> + >> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >> | PPMU_PMNC_COUNTER_RESET_MASK >> | PPMU_PMNC_CC_RESET_MASK >> @@ -284,7 +400,10 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >> pmnc |= (PPMU_V2_MODE_MANUAL << PPMU_V2_PMNC_START_MODE_SHIFT); >> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >> + >> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >> + if (ret < 0) >> + return ret; >> >> return 0; >> } >> @@ -294,37 +413,61 @@ static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev, >> { >> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >> int id = exynos_ppmu_find_ppmu_id(edev); >> - u32 pmnc, cntenc; >> - u32 pmcnt_high, pmcnt_low; >> - u64 load_count = 0; >> + int ret; >> + unsigned int pmnc, cntenc; >> + unsigned int pmcnt_high, pmcnt_low; >> + unsigned int total_count, count; >> + unsigned long load_count = 0; >> >> /* Disable PPMU */ >> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >> + if (ret < 0) >> + return ret; >> + >> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >> + if (ret < 0) >> + return ret; >> >> /* Read cycle count and performance count */ >> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_V2_CCNT); >> + ret = regmap_read(info->regmap, PPMU_V2_CCNT, &total_count); >> + if (ret < 0) >> + return ret; >> + edata->total_count = total_count; >> >> switch (id) { >> case PPMU_PMNCNT0: >> case PPMU_PMNCNT1: >> case PPMU_PMNCNT2: >> - load_count = __raw_readl(info->ppmu.base + PPMU_V2_PMNCT(id)); >> + ret = regmap_read(info->regmap, PPMU_V2_PMNCT(id), &count); >> + if (ret < 0) >> + return ret; >> + load_count = count; >> break; >> case PPMU_PMNCNT3: >> - pmcnt_high = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_HIGH); >> - pmcnt_low = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_LOW); >> - load_count = ((u64)((pmcnt_high & 0xff)) << 32) >> - + (u64)pmcnt_low; >> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_HIGH, >> + &pmcnt_high); >> + if (ret < 0) >> + return ret; >> + >> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_LOW, &pmcnt_low); >> + if (ret < 0) >> + return ret; >> + >> + load_count = ((u64)((pmcnt_high & 0xff)) << 32)+ (u64)pmcnt_low; >> break; >> } >> edata->load_count = load_count; >> >> /* Disable all counters */ >> - cntenc = __raw_readl(info->ppmu.base + PPMU_V2_CNTENC); >> + ret = regmap_read(info->regmap, PPMU_V2_CNTENC, &cntenc); >> + if (ret < 0) >> + return 0; >> + >> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >> - __raw_writel(cntenc, info->ppmu.base + PPMU_V2_CNTENC); >> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, cntenc); >> + if (ret < 0) >> + return ret; >> >> dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name, >> edata->load_count, edata->total_count); >> @@ -411,10 +554,19 @@ static int of_get_devfreq_events(struct device_node *np, >> return 0; >> } >> >> -static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >> +static struct regmap_config exynos_ppmu_regmap_config = { >> + .reg_bits = 32, >> + .val_bits = 32, >> + .reg_stride = 4, >> +}; >> + >> +static int exynos_ppmu_parse_dt(struct platform_device *pdev, >> + struct exynos_ppmu *info) >> { >> struct device *dev = info->dev; >> struct device_node *np = dev->of_node; >> + struct resource *res; >> + void __iomem *base; >> int ret = 0; >> >> if (!np) { >> @@ -423,10 +575,17 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >> } >> >> /* Maps the memory mapped IO to control PPMU register */ >> - info->ppmu.base = of_iomap(np, 0); >> - if (IS_ERR_OR_NULL(info->ppmu.base)) { >> - dev_err(dev, "failed to map memory region\n"); >> - return -ENOMEM; >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + exynos_ppmu_regmap_config.max_register = resource_size(res) - 4; >> + info->regmap = devm_regmap_init_mmio(dev, base, >> + &exynos_ppmu_regmap_config); >> + if (IS_ERR(info->regmap)) { >> + dev_err(dev, "failed to initialize regmap\n"); >> + return PTR_ERR(info->regmap); >> } >> >> info->ppmu.clk = devm_clk_get(dev, "ppmu"); >> @@ -438,15 +597,10 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >> ret = of_get_devfreq_events(np, info); >> if (ret < 0) { >> dev_err(dev, "failed to parse exynos ppmu dt node\n"); >> - goto err; >> + return ret; >> } >> >> return 0; >> - >> -err: >> - iounmap(info->ppmu.base); >> - >> - return ret; >> } >> >> static int exynos_ppmu_probe(struct platform_device *pdev) >> @@ -463,7 +617,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >> info->dev = &pdev->dev; >> >> /* Parse dt data to get resource */ >> - ret = exynos_ppmu_parse_dt(info); >> + ret = exynos_ppmu_parse_dt(pdev, info); >> if (ret < 0) { >> dev_err(&pdev->dev, >> "failed to parse devicetree for resource\n"); >> @@ -476,8 +630,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >> if (!info->edev) { >> dev_err(&pdev->dev, >> "failed to allocate memory devfreq-event devices\n"); >> - ret = -ENOMEM; >> - goto err; >> + return -ENOMEM; >> } >> edev = info->edev; >> platform_set_drvdata(pdev, info); >> @@ -488,17 +641,13 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >> ret = PTR_ERR(edev[i]); >> dev_err(&pdev->dev, >> "failed to add devfreq-event device\n"); >> - goto err; >> + return PTR_ERR(edev[i]); >> } >> } >> >> clk_prepare_enable(info->ppmu.clk); >> >> return 0; >> -err: >> - iounmap(info->ppmu.base); >> - >> - return ret; >> } >> >> static int exynos_ppmu_remove(struct platform_device *pdev) >> @@ -506,7 +655,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev) >> struct exynos_ppmu *info = platform_get_drvdata(pdev); >> >> clk_disable_unprepare(info->ppmu.clk); >> - iounmap(info->ppmu.base); >> >> return 0; >> } >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-20 2:35 ` Chanwoo Choi @ 2016-12-20 8:08 ` Tobias Jakobi 2016-12-20 8:26 ` Chanwoo Choi 0 siblings, 1 reply; 17+ messages in thread From: Tobias Jakobi @ 2016-12-20 8:08 UTC (permalink / raw) To: Chanwoo Choi, myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc Hello Chanwoo, Chanwoo Choi wrote: > Hi, > > On 2016년 12월 20일 04:47, Tobias Jakobi wrote: >> Hello, >> >> I was just wondering what is improved by moving to regmap. For me this >> looks like it only complicates the code. Lots of regmap_{read,write}() >> and for each one of these we need to check the return code. > > It is correct to check the return value. It cover all of exception. that doesn't really answer my question. Which 'exceptions' are we talking about? What can go wrong with __raw_{writel,readl}(), that makes it necessary to put another layer on top of it? AFAIK regmap was introduced to handle read/writes to slow busses like I2C and SPI. I don't see that this applies here. >> Also when exactly did __raw_writel() and friends become legacy? > > Also, I want to use the regmap interface (regmap-io) instead of __raw_readl/__raw_writel using the base address > directly. I see, but why are __raw_{writel,readl}() legacy as you say? I don't see this anywhere else in the kernel. With best wishes, Tobias > > Regards, > Chanwoo Choi > >> >> With best wishes, >> Tobias >> >> >> Chanwoo Choi wrote: >>> This patch uses the regmap interface to read and write the registers for exynos >>> PPMU device instead of the legacy memory map functions. >>> >>> Cc: Kukjin Kim <kgene@kernel.org> >>> Cc: Krzysztof Kozlowski <krzk@kernel.org> >>> Cc: Javier Martinez Canillas <javier@osg.samsung.com> >>> Cc: linux-samsung-soc@vger.kernel.org >>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>> --- >>> drivers/devfreq/event/exynos-ppmu.c | 326 ++++++++++++++++++++++++++---------- >>> 1 file changed, 237 insertions(+), 89 deletions(-) >>> >>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c >>> index 107eb91a9415..fb3706faf5bd 100644 >>> --- a/drivers/devfreq/event/exynos-ppmu.c >>> +++ b/drivers/devfreq/event/exynos-ppmu.c >>> @@ -17,13 +17,13 @@ >>> #include <linux/module.h> >>> #include <linux/of_address.h> >>> #include <linux/platform_device.h> >>> +#include <linux/regmap.h> >>> #include <linux/suspend.h> >>> #include <linux/devfreq-event.h> >>> >>> #include "exynos-ppmu.h" >>> >>> struct exynos_ppmu_data { >>> - void __iomem *base; >>> struct clk *clk; >>> }; >>> >>> @@ -33,6 +33,7 @@ struct exynos_ppmu { >>> unsigned int num_events; >>> >>> struct device *dev; >>> + struct regmap *regmap; >>> >>> struct exynos_ppmu_data ppmu; >>> }; >>> @@ -107,20 +108,28 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev) >>> static int exynos_ppmu_disable(struct devfreq_event_dev *edev) >>> { >>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>> + int ret; >>> u32 pmnc; >>> >>> /* Disable all counters */ >>> - __raw_writel(PPMU_CCNT_MASK | >>> - PPMU_PMCNT0_MASK | >>> - PPMU_PMCNT1_MASK | >>> - PPMU_PMCNT2_MASK | >>> - PPMU_PMCNT3_MASK, >>> - info->ppmu.base + PPMU_CNTENC); >>> + ret = regmap_write(info->regmap, PPMU_CNTENC, >>> + PPMU_CCNT_MASK | >>> + PPMU_PMCNT0_MASK | >>> + PPMU_PMCNT1_MASK | >>> + PPMU_PMCNT2_MASK | >>> + PPMU_PMCNT3_MASK); >>> + if (ret < 0) >>> + return ret; >>> >>> /* Disable PPMU */ >>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>> + if (ret < 0) >>> + return ret; >>> + >>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>> + if (ret < 0) >>> + return ret; >>> >>> return 0; >>> } >>> @@ -129,29 +138,42 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev) >>> { >>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>> int id = exynos_ppmu_find_ppmu_id(edev); >>> + int ret; >>> u32 pmnc, cntens; >>> >>> if (id < 0) >>> return id; >>> >>> /* Enable specific counter */ >>> - cntens = __raw_readl(info->ppmu.base + PPMU_CNTENS); >>> + ret = regmap_read(info->regmap, PPMU_CNTENS, &cntens); >>> + if (ret < 0) >>> + return ret; >>> + >>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>> - __raw_writel(cntens, info->ppmu.base + PPMU_CNTENS); >>> + ret = regmap_write(info->regmap, PPMU_CNTENS, cntens); >>> + if (ret < 0) >>> + return ret; >>> >>> /* Set the event of Read/Write data count */ >>> - __raw_writel(PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT, >>> - info->ppmu.base + PPMU_BEVTxSEL(id)); >>> + ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), >>> + PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT); >>> + if (ret < 0) >>> + return ret; >>> >>> /* Reset cycle counter/performance counter and enable PPMU */ >>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>> + if (ret < 0) >>> + return ret; >>> + >>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>> | PPMU_PMNC_COUNTER_RESET_MASK >>> | PPMU_PMNC_CC_RESET_MASK); >>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT); >>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>> + if (ret < 0) >>> + return ret; >>> >>> return 0; >>> } >>> @@ -161,40 +183,64 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>> { >>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>> int id = exynos_ppmu_find_ppmu_id(edev); >>> - u32 pmnc, cntenc; >>> + unsigned int total_count, load_count; >>> + unsigned int pmcnt3_high, pmcnt3_low; >>> + unsigned int pmnc, cntenc; >>> + int ret; >>> >>> if (id < 0) >>> return -EINVAL; >>> >>> /* Disable PPMU */ >>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>> + if (ret < 0) >>> + return ret; >>> + >>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>> + if (ret < 0) >>> + return ret; >>> >>> /* Read cycle count */ >>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_CCNT); >>> + ret = regmap_read(info->regmap, PPMU_CCNT, &total_count); >>> + if (ret < 0) >>> + return ret; >>> + edata->total_count = total_count; >>> >>> /* Read performance count */ >>> switch (id) { >>> case PPMU_PMNCNT0: >>> case PPMU_PMNCNT1: >>> case PPMU_PMNCNT2: >>> - edata->load_count >>> - = __raw_readl(info->ppmu.base + PPMU_PMNCT(id)); >>> + ret = regmap_read(info->regmap, PPMU_PMNCT(id), &load_count); >>> + if (ret < 0) >>> + return ret; >>> + edata->load_count = load_count; >>> break; >>> case PPMU_PMNCNT3: >>> - edata->load_count = >>> - ((__raw_readl(info->ppmu.base + PPMU_PMCNT3_HIGH) << 8) >>> - | __raw_readl(info->ppmu.base + PPMU_PMCNT3_LOW)); >>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_HIGH, &pmcnt3_high); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_LOW, &pmcnt3_low); >>> + if (ret < 0) >>> + return ret; >>> + >>> + edata->load_count = ((pmcnt3_high << 8) | pmcnt3_low); >>> break; >>> default: >>> return -EINVAL; >>> } >>> >>> /* Disable specific counter */ >>> - cntenc = __raw_readl(info->ppmu.base + PPMU_CNTENC); >>> + ret = regmap_read(info->regmap, PPMU_CNTENC, &cntenc); >>> + if (ret < 0) >>> + return ret; >>> + >>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>> - __raw_writel(cntenc, info->ppmu.base + PPMU_CNTENC); >>> + ret = regmap_write(info->regmap, PPMU_CNTENC, cntenc); >>> + if (ret < 0) >>> + return ret; >>> >>> dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name, >>> edata->load_count, edata->total_count); >>> @@ -214,36 +260,93 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>> static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>> { >>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>> + int ret; >>> u32 pmnc, clear; >>> >>> /* Disable all counters */ >>> clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK >>> | PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK); >>> + ret = regmap_write(info->regmap, PPMU_V2_FLAG, clear); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_INTENC, clear); >>> + if (ret < 0) >>> + return ret; >>> >>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_FLAG); >>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_INTENC); >>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNTENC); >>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNT_RESET); >>> - >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG0); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG1); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG2); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_RESULT); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CNT_AUTO); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV0_TYPE); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV1_TYPE); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV2_TYPE); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV3_TYPE); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_V); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_A); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_V); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_A); >>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_INTERRUPT_RESET); >>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, clear); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_RESET, clear); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG0, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG1, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG2, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_RESULT, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_AUTO, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV0_TYPE, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV1_TYPE, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV2_TYPE, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV3_TYPE, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_V, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_A, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_V, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_A, 0x0); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_INTERRUPT_RESET, 0x0); >>> + if (ret < 0) >>> + return ret; >>> >>> /* Disable PPMU */ >>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>> + if (ret < 0) >>> + return ret; >>> + >>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>> + if (ret < 0) >>> + return ret; >>> >>> return 0; >>> } >>> @@ -251,30 +354,43 @@ static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>> static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>> { >>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>> + unsigned int pmnc, cntens; >>> int id = exynos_ppmu_find_ppmu_id(edev); >>> - u32 pmnc, cntens; >>> + int ret; >>> >>> /* Enable all counters */ >>> - cntens = __raw_readl(info->ppmu.base + PPMU_V2_CNTENS); >>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens); >>> + if (ret < 0) >>> + return ret; >>> + >>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>> - __raw_writel(cntens, info->ppmu.base + PPMU_V2_CNTENS); >>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENS, cntens); >>> + if (ret < 0) >>> + return ret; >>> >>> /* Set the event of Read/Write data count */ >>> switch (id) { >>> case PPMU_PMNCNT0: >>> case PPMU_PMNCNT1: >>> case PPMU_PMNCNT2: >>> - __raw_writel(PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT, >>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>> + PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT); >>> + if (ret < 0) >>> + return ret; >>> break; >>> case PPMU_PMNCNT3: >>> - __raw_writel(PPMU_V2_EVT3_RW_DATA_CNT, >>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>> + PPMU_V2_EVT3_RW_DATA_CNT); >>> + if (ret < 0) >>> + return ret; >>> break; >>> } >>> >>> /* Reset cycle counter/performance counter and enable PPMU */ >>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>> + if (ret < 0) >>> + return ret; >>> + >>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>> | PPMU_PMNC_COUNTER_RESET_MASK >>> | PPMU_PMNC_CC_RESET_MASK >>> @@ -284,7 +400,10 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>> pmnc |= (PPMU_V2_MODE_MANUAL << PPMU_V2_PMNC_START_MODE_SHIFT); >>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>> + >>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>> + if (ret < 0) >>> + return ret; >>> >>> return 0; >>> } >>> @@ -294,37 +413,61 @@ static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev, >>> { >>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>> int id = exynos_ppmu_find_ppmu_id(edev); >>> - u32 pmnc, cntenc; >>> - u32 pmcnt_high, pmcnt_low; >>> - u64 load_count = 0; >>> + int ret; >>> + unsigned int pmnc, cntenc; >>> + unsigned int pmcnt_high, pmcnt_low; >>> + unsigned int total_count, count; >>> + unsigned long load_count = 0; >>> >>> /* Disable PPMU */ >>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>> + if (ret < 0) >>> + return ret; >>> + >>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>> + if (ret < 0) >>> + return ret; >>> >>> /* Read cycle count and performance count */ >>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_V2_CCNT); >>> + ret = regmap_read(info->regmap, PPMU_V2_CCNT, &total_count); >>> + if (ret < 0) >>> + return ret; >>> + edata->total_count = total_count; >>> >>> switch (id) { >>> case PPMU_PMNCNT0: >>> case PPMU_PMNCNT1: >>> case PPMU_PMNCNT2: >>> - load_count = __raw_readl(info->ppmu.base + PPMU_V2_PMNCT(id)); >>> + ret = regmap_read(info->regmap, PPMU_V2_PMNCT(id), &count); >>> + if (ret < 0) >>> + return ret; >>> + load_count = count; >>> break; >>> case PPMU_PMNCNT3: >>> - pmcnt_high = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_HIGH); >>> - pmcnt_low = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_LOW); >>> - load_count = ((u64)((pmcnt_high & 0xff)) << 32) >>> - + (u64)pmcnt_low; >>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_HIGH, >>> + &pmcnt_high); >>> + if (ret < 0) >>> + return ret; >>> + >>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_LOW, &pmcnt_low); >>> + if (ret < 0) >>> + return ret; >>> + >>> + load_count = ((u64)((pmcnt_high & 0xff)) << 32)+ (u64)pmcnt_low; >>> break; >>> } >>> edata->load_count = load_count; >>> >>> /* Disable all counters */ >>> - cntenc = __raw_readl(info->ppmu.base + PPMU_V2_CNTENC); >>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENC, &cntenc); >>> + if (ret < 0) >>> + return 0; >>> + >>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>> - __raw_writel(cntenc, info->ppmu.base + PPMU_V2_CNTENC); >>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, cntenc); >>> + if (ret < 0) >>> + return ret; >>> >>> dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name, >>> edata->load_count, edata->total_count); >>> @@ -411,10 +554,19 @@ static int of_get_devfreq_events(struct device_node *np, >>> return 0; >>> } >>> >>> -static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>> +static struct regmap_config exynos_ppmu_regmap_config = { >>> + .reg_bits = 32, >>> + .val_bits = 32, >>> + .reg_stride = 4, >>> +}; >>> + >>> +static int exynos_ppmu_parse_dt(struct platform_device *pdev, >>> + struct exynos_ppmu *info) >>> { >>> struct device *dev = info->dev; >>> struct device_node *np = dev->of_node; >>> + struct resource *res; >>> + void __iomem *base; >>> int ret = 0; >>> >>> if (!np) { >>> @@ -423,10 +575,17 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>> } >>> >>> /* Maps the memory mapped IO to control PPMU register */ >>> - info->ppmu.base = of_iomap(np, 0); >>> - if (IS_ERR_OR_NULL(info->ppmu.base)) { >>> - dev_err(dev, "failed to map memory region\n"); >>> - return -ENOMEM; >>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>> + base = devm_ioremap_resource(dev, res); >>> + if (IS_ERR(base)) >>> + return PTR_ERR(base); >>> + >>> + exynos_ppmu_regmap_config.max_register = resource_size(res) - 4; >>> + info->regmap = devm_regmap_init_mmio(dev, base, >>> + &exynos_ppmu_regmap_config); >>> + if (IS_ERR(info->regmap)) { >>> + dev_err(dev, "failed to initialize regmap\n"); >>> + return PTR_ERR(info->regmap); >>> } >>> >>> info->ppmu.clk = devm_clk_get(dev, "ppmu"); >>> @@ -438,15 +597,10 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>> ret = of_get_devfreq_events(np, info); >>> if (ret < 0) { >>> dev_err(dev, "failed to parse exynos ppmu dt node\n"); >>> - goto err; >>> + return ret; >>> } >>> >>> return 0; >>> - >>> -err: >>> - iounmap(info->ppmu.base); >>> - >>> - return ret; >>> } >>> >>> static int exynos_ppmu_probe(struct platform_device *pdev) >>> @@ -463,7 +617,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>> info->dev = &pdev->dev; >>> >>> /* Parse dt data to get resource */ >>> - ret = exynos_ppmu_parse_dt(info); >>> + ret = exynos_ppmu_parse_dt(pdev, info); >>> if (ret < 0) { >>> dev_err(&pdev->dev, >>> "failed to parse devicetree for resource\n"); >>> @@ -476,8 +630,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>> if (!info->edev) { >>> dev_err(&pdev->dev, >>> "failed to allocate memory devfreq-event devices\n"); >>> - ret = -ENOMEM; >>> - goto err; >>> + return -ENOMEM; >>> } >>> edev = info->edev; >>> platform_set_drvdata(pdev, info); >>> @@ -488,17 +641,13 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>> ret = PTR_ERR(edev[i]); >>> dev_err(&pdev->dev, >>> "failed to add devfreq-event device\n"); >>> - goto err; >>> + return PTR_ERR(edev[i]); >>> } >>> } >>> >>> clk_prepare_enable(info->ppmu.clk); >>> >>> return 0; >>> -err: >>> - iounmap(info->ppmu.base); >>> - >>> - return ret; >>> } >>> >>> static int exynos_ppmu_remove(struct platform_device *pdev) >>> @@ -506,7 +655,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev) >>> struct exynos_ppmu *info = platform_get_drvdata(pdev); >>> >>> clk_disable_unprepare(info->ppmu.clk); >>> - iounmap(info->ppmu.base); >>> >>> return 0; >>> } >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-20 8:08 ` Tobias Jakobi @ 2016-12-20 8:26 ` Chanwoo Choi 2016-12-20 8:32 ` Chanwoo Choi 2016-12-20 13:47 ` Tobias Jakobi 0 siblings, 2 replies; 17+ messages in thread From: Chanwoo Choi @ 2016-12-20 8:26 UTC (permalink / raw) To: Tobias Jakobi, myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc On 2016년 12월 20일 17:08, Tobias Jakobi wrote: > Hello Chanwoo, > > > Chanwoo Choi wrote: >> Hi, >> >> On 2016년 12월 20일 04:47, Tobias Jakobi wrote: >>> Hello, >>> >>> I was just wondering what is improved by moving to regmap. For me this >>> looks like it only complicates the code. Lots of regmap_{read,write}() >>> and for each one of these we need to check the return code. >> >> It is correct to check the return value. It cover all of exception. > that doesn't really answer my question. Which 'exceptions' are we > talking about? What can go wrong with __raw_{writel,readl}(), that When using __raw_readl/writel() don't check the any return value, it it not correct. When calling the function, basically we should check whether return value is error or success. What is problem to check the return value? > makes it necessary to put another layer on top of it? AFAIK regmap was > introduced to handle read/writes to slow busses like I2C and SPI. I > don't see that this applies here. The regmap support the MMIO on following driver. - drivers/base/regmap/regmap-mmio.c > > >>> Also when exactly did __raw_writel() and friends become legacy? >> >> Also, I want to use the regmap interface (regmap-io) instead of __raw_readl/__raw_writel using the base address >> directly. > I see, but why are __raw_{writel,readl}() legacy as you say? I don't see > this anywhere else in the kernel. If you just don't like the 'legacy' expression. I'll remove it. It is not any important. The key point of this patch uses the regmap interface. Usually, when adding new device driver, I use the regmap mmio interface instead of __raw_readl/writel. So, this patch changes it. Regards, Chanwoo Choi > > > With best wishes, > Tobias > > >> >> Regards, >> Chanwoo Choi >> >>> >>> With best wishes, >>> Tobias >>> >>> >>> Chanwoo Choi wrote: >>>> This patch uses the regmap interface to read and write the registers for exynos >>>> PPMU device instead of the legacy memory map functions. >>>> >>>> Cc: Kukjin Kim <kgene@kernel.org> >>>> Cc: Krzysztof Kozlowski <krzk@kernel.org> >>>> Cc: Javier Martinez Canillas <javier@osg.samsung.com> >>>> Cc: linux-samsung-soc@vger.kernel.org >>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>>> --- >>>> drivers/devfreq/event/exynos-ppmu.c | 326 ++++++++++++++++++++++++++---------- >>>> 1 file changed, 237 insertions(+), 89 deletions(-) >>>> >>>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c >>>> index 107eb91a9415..fb3706faf5bd 100644 >>>> --- a/drivers/devfreq/event/exynos-ppmu.c >>>> +++ b/drivers/devfreq/event/exynos-ppmu.c >>>> @@ -17,13 +17,13 @@ >>>> #include <linux/module.h> >>>> #include <linux/of_address.h> >>>> #include <linux/platform_device.h> >>>> +#include <linux/regmap.h> >>>> #include <linux/suspend.h> >>>> #include <linux/devfreq-event.h> >>>> >>>> #include "exynos-ppmu.h" >>>> >>>> struct exynos_ppmu_data { >>>> - void __iomem *base; >>>> struct clk *clk; >>>> }; >>>> >>>> @@ -33,6 +33,7 @@ struct exynos_ppmu { >>>> unsigned int num_events; >>>> >>>> struct device *dev; >>>> + struct regmap *regmap; >>>> >>>> struct exynos_ppmu_data ppmu; >>>> }; >>>> @@ -107,20 +108,28 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev) >>>> static int exynos_ppmu_disable(struct devfreq_event_dev *edev) >>>> { >>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>> + int ret; >>>> u32 pmnc; >>>> >>>> /* Disable all counters */ >>>> - __raw_writel(PPMU_CCNT_MASK | >>>> - PPMU_PMCNT0_MASK | >>>> - PPMU_PMCNT1_MASK | >>>> - PPMU_PMCNT2_MASK | >>>> - PPMU_PMCNT3_MASK, >>>> - info->ppmu.base + PPMU_CNTENC); >>>> + ret = regmap_write(info->regmap, PPMU_CNTENC, >>>> + PPMU_CCNT_MASK | >>>> + PPMU_PMCNT0_MASK | >>>> + PPMU_PMCNT1_MASK | >>>> + PPMU_PMCNT2_MASK | >>>> + PPMU_PMCNT3_MASK); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> /* Disable PPMU */ >>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> return 0; >>>> } >>>> @@ -129,29 +138,42 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev) >>>> { >>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>> + int ret; >>>> u32 pmnc, cntens; >>>> >>>> if (id < 0) >>>> return id; >>>> >>>> /* Enable specific counter */ >>>> - cntens = __raw_readl(info->ppmu.base + PPMU_CNTENS); >>>> + ret = regmap_read(info->regmap, PPMU_CNTENS, &cntens); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>> - __raw_writel(cntens, info->ppmu.base + PPMU_CNTENS); >>>> + ret = regmap_write(info->regmap, PPMU_CNTENS, cntens); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> /* Set the event of Read/Write data count */ >>>> - __raw_writel(PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT, >>>> - info->ppmu.base + PPMU_BEVTxSEL(id)); >>>> + ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), >>>> + PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> /* Reset cycle counter/performance counter and enable PPMU */ >>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>>> | PPMU_PMNC_COUNTER_RESET_MASK >>>> | PPMU_PMNC_CC_RESET_MASK); >>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT); >>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> return 0; >>>> } >>>> @@ -161,40 +183,64 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>>> { >>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>> - u32 pmnc, cntenc; >>>> + unsigned int total_count, load_count; >>>> + unsigned int pmcnt3_high, pmcnt3_low; >>>> + unsigned int pmnc, cntenc; >>>> + int ret; >>>> >>>> if (id < 0) >>>> return -EINVAL; >>>> >>>> /* Disable PPMU */ >>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> /* Read cycle count */ >>>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_CCNT); >>>> + ret = regmap_read(info->regmap, PPMU_CCNT, &total_count); >>>> + if (ret < 0) >>>> + return ret; >>>> + edata->total_count = total_count; >>>> >>>> /* Read performance count */ >>>> switch (id) { >>>> case PPMU_PMNCNT0: >>>> case PPMU_PMNCNT1: >>>> case PPMU_PMNCNT2: >>>> - edata->load_count >>>> - = __raw_readl(info->ppmu.base + PPMU_PMNCT(id)); >>>> + ret = regmap_read(info->regmap, PPMU_PMNCT(id), &load_count); >>>> + if (ret < 0) >>>> + return ret; >>>> + edata->load_count = load_count; >>>> break; >>>> case PPMU_PMNCNT3: >>>> - edata->load_count = >>>> - ((__raw_readl(info->ppmu.base + PPMU_PMCNT3_HIGH) << 8) >>>> - | __raw_readl(info->ppmu.base + PPMU_PMCNT3_LOW)); >>>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_HIGH, &pmcnt3_high); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_LOW, &pmcnt3_low); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + edata->load_count = ((pmcnt3_high << 8) | pmcnt3_low); >>>> break; >>>> default: >>>> return -EINVAL; >>>> } >>>> >>>> /* Disable specific counter */ >>>> - cntenc = __raw_readl(info->ppmu.base + PPMU_CNTENC); >>>> + ret = regmap_read(info->regmap, PPMU_CNTENC, &cntenc); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>> - __raw_writel(cntenc, info->ppmu.base + PPMU_CNTENC); >>>> + ret = regmap_write(info->regmap, PPMU_CNTENC, cntenc); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name, >>>> edata->load_count, edata->total_count); >>>> @@ -214,36 +260,93 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>>> static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>>> { >>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>> + int ret; >>>> u32 pmnc, clear; >>>> >>>> /* Disable all counters */ >>>> clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK >>>> | PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK); >>>> + ret = regmap_write(info->regmap, PPMU_V2_FLAG, clear); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_INTENC, clear); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_FLAG); >>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_INTENC); >>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNTENC); >>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNT_RESET); >>>> - >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG0); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG1); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG2); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_RESULT); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CNT_AUTO); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV0_TYPE); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV1_TYPE); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV2_TYPE); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV3_TYPE); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_V); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_A); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_V); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_A); >>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_INTERRUPT_RESET); >>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, clear); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_RESET, clear); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG0, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG1, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG2, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_RESULT, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_AUTO, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV0_TYPE, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV1_TYPE, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV2_TYPE, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV3_TYPE, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_V, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_A, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_V, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_A, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_INTERRUPT_RESET, 0x0); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> /* Disable PPMU */ >>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> return 0; >>>> } >>>> @@ -251,30 +354,43 @@ static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>>> static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>>> { >>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>> + unsigned int pmnc, cntens; >>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>> - u32 pmnc, cntens; >>>> + int ret; >>>> >>>> /* Enable all counters */ >>>> - cntens = __raw_readl(info->ppmu.base + PPMU_V2_CNTENS); >>>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>> - __raw_writel(cntens, info->ppmu.base + PPMU_V2_CNTENS); >>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENS, cntens); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> /* Set the event of Read/Write data count */ >>>> switch (id) { >>>> case PPMU_PMNCNT0: >>>> case PPMU_PMNCNT1: >>>> case PPMU_PMNCNT2: >>>> - __raw_writel(PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT, >>>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>>> + PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT); >>>> + if (ret < 0) >>>> + return ret; >>>> break; >>>> case PPMU_PMNCNT3: >>>> - __raw_writel(PPMU_V2_EVT3_RW_DATA_CNT, >>>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>>> + PPMU_V2_EVT3_RW_DATA_CNT); >>>> + if (ret < 0) >>>> + return ret; >>>> break; >>>> } >>>> >>>> /* Reset cycle counter/performance counter and enable PPMU */ >>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>>> | PPMU_PMNC_COUNTER_RESET_MASK >>>> | PPMU_PMNC_CC_RESET_MASK >>>> @@ -284,7 +400,10 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>>> pmnc |= (PPMU_V2_MODE_MANUAL << PPMU_V2_PMNC_START_MODE_SHIFT); >>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>> + >>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> return 0; >>>> } >>>> @@ -294,37 +413,61 @@ static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev, >>>> { >>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>> - u32 pmnc, cntenc; >>>> - u32 pmcnt_high, pmcnt_low; >>>> - u64 load_count = 0; >>>> + int ret; >>>> + unsigned int pmnc, cntenc; >>>> + unsigned int pmcnt_high, pmcnt_low; >>>> + unsigned int total_count, count; >>>> + unsigned long load_count = 0; >>>> >>>> /* Disable PPMU */ >>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> /* Read cycle count and performance count */ >>>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_V2_CCNT); >>>> + ret = regmap_read(info->regmap, PPMU_V2_CCNT, &total_count); >>>> + if (ret < 0) >>>> + return ret; >>>> + edata->total_count = total_count; >>>> >>>> switch (id) { >>>> case PPMU_PMNCNT0: >>>> case PPMU_PMNCNT1: >>>> case PPMU_PMNCNT2: >>>> - load_count = __raw_readl(info->ppmu.base + PPMU_V2_PMNCT(id)); >>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNCT(id), &count); >>>> + if (ret < 0) >>>> + return ret; >>>> + load_count = count; >>>> break; >>>> case PPMU_PMNCNT3: >>>> - pmcnt_high = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_HIGH); >>>> - pmcnt_low = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_LOW); >>>> - load_count = ((u64)((pmcnt_high & 0xff)) << 32) >>>> - + (u64)pmcnt_low; >>>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_HIGH, >>>> + &pmcnt_high); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_LOW, &pmcnt_low); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + load_count = ((u64)((pmcnt_high & 0xff)) << 32)+ (u64)pmcnt_low; >>>> break; >>>> } >>>> edata->load_count = load_count; >>>> >>>> /* Disable all counters */ >>>> - cntenc = __raw_readl(info->ppmu.base + PPMU_V2_CNTENC); >>>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENC, &cntenc); >>>> + if (ret < 0) >>>> + return 0; >>>> + >>>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>> - __raw_writel(cntenc, info->ppmu.base + PPMU_V2_CNTENC); >>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, cntenc); >>>> + if (ret < 0) >>>> + return ret; >>>> >>>> dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name, >>>> edata->load_count, edata->total_count); >>>> @@ -411,10 +554,19 @@ static int of_get_devfreq_events(struct device_node *np, >>>> return 0; >>>> } >>>> >>>> -static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>> +static struct regmap_config exynos_ppmu_regmap_config = { >>>> + .reg_bits = 32, >>>> + .val_bits = 32, >>>> + .reg_stride = 4, >>>> +}; >>>> + >>>> +static int exynos_ppmu_parse_dt(struct platform_device *pdev, >>>> + struct exynos_ppmu *info) >>>> { >>>> struct device *dev = info->dev; >>>> struct device_node *np = dev->of_node; >>>> + struct resource *res; >>>> + void __iomem *base; >>>> int ret = 0; >>>> >>>> if (!np) { >>>> @@ -423,10 +575,17 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>> } >>>> >>>> /* Maps the memory mapped IO to control PPMU register */ >>>> - info->ppmu.base = of_iomap(np, 0); >>>> - if (IS_ERR_OR_NULL(info->ppmu.base)) { >>>> - dev_err(dev, "failed to map memory region\n"); >>>> - return -ENOMEM; >>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>> + base = devm_ioremap_resource(dev, res); >>>> + if (IS_ERR(base)) >>>> + return PTR_ERR(base); >>>> + >>>> + exynos_ppmu_regmap_config.max_register = resource_size(res) - 4; >>>> + info->regmap = devm_regmap_init_mmio(dev, base, >>>> + &exynos_ppmu_regmap_config); >>>> + if (IS_ERR(info->regmap)) { >>>> + dev_err(dev, "failed to initialize regmap\n"); >>>> + return PTR_ERR(info->regmap); >>>> } >>>> >>>> info->ppmu.clk = devm_clk_get(dev, "ppmu"); >>>> @@ -438,15 +597,10 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>> ret = of_get_devfreq_events(np, info); >>>> if (ret < 0) { >>>> dev_err(dev, "failed to parse exynos ppmu dt node\n"); >>>> - goto err; >>>> + return ret; >>>> } >>>> >>>> return 0; >>>> - >>>> -err: >>>> - iounmap(info->ppmu.base); >>>> - >>>> - return ret; >>>> } >>>> >>>> static int exynos_ppmu_probe(struct platform_device *pdev) >>>> @@ -463,7 +617,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>> info->dev = &pdev->dev; >>>> >>>> /* Parse dt data to get resource */ >>>> - ret = exynos_ppmu_parse_dt(info); >>>> + ret = exynos_ppmu_parse_dt(pdev, info); >>>> if (ret < 0) { >>>> dev_err(&pdev->dev, >>>> "failed to parse devicetree for resource\n"); >>>> @@ -476,8 +630,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>> if (!info->edev) { >>>> dev_err(&pdev->dev, >>>> "failed to allocate memory devfreq-event devices\n"); >>>> - ret = -ENOMEM; >>>> - goto err; >>>> + return -ENOMEM; >>>> } >>>> edev = info->edev; >>>> platform_set_drvdata(pdev, info); >>>> @@ -488,17 +641,13 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>> ret = PTR_ERR(edev[i]); >>>> dev_err(&pdev->dev, >>>> "failed to add devfreq-event device\n"); >>>> - goto err; >>>> + return PTR_ERR(edev[i]); >>>> } >>>> } >>>> >>>> clk_prepare_enable(info->ppmu.clk); >>>> >>>> return 0; >>>> -err: >>>> - iounmap(info->ppmu.base); >>>> - >>>> - return ret; >>>> } >>>> >>>> static int exynos_ppmu_remove(struct platform_device *pdev) >>>> @@ -506,7 +655,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev) >>>> struct exynos_ppmu *info = platform_get_drvdata(pdev); >>>> >>>> clk_disable_unprepare(info->ppmu.clk); >>>> - iounmap(info->ppmu.base); >>>> >>>> return 0; >>>> } >>>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-20 8:26 ` Chanwoo Choi @ 2016-12-20 8:32 ` Chanwoo Choi 2016-12-20 13:47 ` Tobias Jakobi 2016-12-20 13:47 ` Tobias Jakobi 1 sibling, 1 reply; 17+ messages in thread From: Chanwoo Choi @ 2016-12-20 8:32 UTC (permalink / raw) To: Tobias Jakobi, myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc On 2016년 12월 20일 17:26, Chanwoo Choi wrote: > On 2016년 12월 20일 17:08, Tobias Jakobi wrote: >> Hello Chanwoo, >> >> >> Chanwoo Choi wrote: >>> Hi, >>> >>> On 2016년 12월 20일 04:47, Tobias Jakobi wrote: >>>> Hello, >>>> >>>> I was just wondering what is improved by moving to regmap. For me this >>>> looks like it only complicates the code. Lots of regmap_{read,write}() >>>> and for each one of these we need to check the return code. >>> >>> It is correct to check the return value. It cover all of exception. >> that doesn't really answer my question. Which 'exceptions' are we >> talking about? What can go wrong with __raw_{writel,readl}(), that > > When using __raw_readl/writel() don't check the any return value, it it not correct. > When calling the function, basically we should check whether return value is error or success. > What is problem to check the return value? I use 'exception' word means the error situations. When handling the register of PPMU, the error might happen. So, we need to check the return value by using the regmap interface. > >> makes it necessary to put another layer on top of it? AFAIK regmap was >> introduced to handle read/writes to slow busses like I2C and SPI. I >> don't see that this applies here. > > The regmap support the MMIO on following driver. > - drivers/base/regmap/regmap-mmio.c > >> >> >>>> Also when exactly did __raw_writel() and friends become legacy? >>> >>> Also, I want to use the regmap interface (regmap-io) instead of __raw_readl/__raw_writel using the base address >>> directly. >> I see, but why are __raw_{writel,readl}() legacy as you say? I don't see >> this anywhere else in the kernel. > > If you just don't like the 'legacy' expression. I'll remove it. > It is not any important. The key point of this patch uses the regmap interface. > Usually, when adding new device driver, I use the regmap mmio interface > instead of __raw_readl/writel. So, this patch changes it. > > Regards, > Chanwoo Choi > >> >> >> With best wishes, >> Tobias >> >> >>> >>> Regards, >>> Chanwoo Choi >>> >>>> >>>> With best wishes, >>>> Tobias >>>> >>>> >>>> Chanwoo Choi wrote: >>>>> This patch uses the regmap interface to read and write the registers for exynos >>>>> PPMU device instead of the legacy memory map functions. >>>>> >>>>> Cc: Kukjin Kim <kgene@kernel.org> >>>>> Cc: Krzysztof Kozlowski <krzk@kernel.org> >>>>> Cc: Javier Martinez Canillas <javier@osg.samsung.com> >>>>> Cc: linux-samsung-soc@vger.kernel.org >>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>>>> --- >>>>> drivers/devfreq/event/exynos-ppmu.c | 326 ++++++++++++++++++++++++++---------- >>>>> 1 file changed, 237 insertions(+), 89 deletions(-) >>>>> >>>>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c >>>>> index 107eb91a9415..fb3706faf5bd 100644 >>>>> --- a/drivers/devfreq/event/exynos-ppmu.c >>>>> +++ b/drivers/devfreq/event/exynos-ppmu.c >>>>> @@ -17,13 +17,13 @@ >>>>> #include <linux/module.h> >>>>> #include <linux/of_address.h> >>>>> #include <linux/platform_device.h> >>>>> +#include <linux/regmap.h> >>>>> #include <linux/suspend.h> >>>>> #include <linux/devfreq-event.h> >>>>> >>>>> #include "exynos-ppmu.h" >>>>> >>>>> struct exynos_ppmu_data { >>>>> - void __iomem *base; >>>>> struct clk *clk; >>>>> }; >>>>> >>>>> @@ -33,6 +33,7 @@ struct exynos_ppmu { >>>>> unsigned int num_events; >>>>> >>>>> struct device *dev; >>>>> + struct regmap *regmap; >>>>> >>>>> struct exynos_ppmu_data ppmu; >>>>> }; >>>>> @@ -107,20 +108,28 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev) >>>>> static int exynos_ppmu_disable(struct devfreq_event_dev *edev) >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> + int ret; >>>>> u32 pmnc; >>>>> >>>>> /* Disable all counters */ >>>>> - __raw_writel(PPMU_CCNT_MASK | >>>>> - PPMU_PMCNT0_MASK | >>>>> - PPMU_PMCNT1_MASK | >>>>> - PPMU_PMCNT2_MASK | >>>>> - PPMU_PMCNT3_MASK, >>>>> - info->ppmu.base + PPMU_CNTENC); >>>>> + ret = regmap_write(info->regmap, PPMU_CNTENC, >>>>> + PPMU_CCNT_MASK | >>>>> + PPMU_PMCNT0_MASK | >>>>> + PPMU_PMCNT1_MASK | >>>>> + PPMU_PMCNT2_MASK | >>>>> + PPMU_PMCNT3_MASK); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Disable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> return 0; >>>>> } >>>>> @@ -129,29 +138,42 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev) >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>> + int ret; >>>>> u32 pmnc, cntens; >>>>> >>>>> if (id < 0) >>>>> return id; >>>>> >>>>> /* Enable specific counter */ >>>>> - cntens = __raw_readl(info->ppmu.base + PPMU_CNTENS); >>>>> + ret = regmap_read(info->regmap, PPMU_CNTENS, &cntens); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>> - __raw_writel(cntens, info->ppmu.base + PPMU_CNTENS); >>>>> + ret = regmap_write(info->regmap, PPMU_CNTENS, cntens); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Set the event of Read/Write data count */ >>>>> - __raw_writel(PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT, >>>>> - info->ppmu.base + PPMU_BEVTxSEL(id)); >>>>> + ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), >>>>> + PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Reset cycle counter/performance counter and enable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>>>> | PPMU_PMNC_COUNTER_RESET_MASK >>>>> | PPMU_PMNC_CC_RESET_MASK); >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT); >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> return 0; >>>>> } >>>>> @@ -161,40 +183,64 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>> - u32 pmnc, cntenc; >>>>> + unsigned int total_count, load_count; >>>>> + unsigned int pmcnt3_high, pmcnt3_low; >>>>> + unsigned int pmnc, cntenc; >>>>> + int ret; >>>>> >>>>> if (id < 0) >>>>> return -EINVAL; >>>>> >>>>> /* Disable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Read cycle count */ >>>>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_CCNT); >>>>> + ret = regmap_read(info->regmap, PPMU_CCNT, &total_count); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + edata->total_count = total_count; >>>>> >>>>> /* Read performance count */ >>>>> switch (id) { >>>>> case PPMU_PMNCNT0: >>>>> case PPMU_PMNCNT1: >>>>> case PPMU_PMNCNT2: >>>>> - edata->load_count >>>>> - = __raw_readl(info->ppmu.base + PPMU_PMNCT(id)); >>>>> + ret = regmap_read(info->regmap, PPMU_PMNCT(id), &load_count); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + edata->load_count = load_count; >>>>> break; >>>>> case PPMU_PMNCNT3: >>>>> - edata->load_count = >>>>> - ((__raw_readl(info->ppmu.base + PPMU_PMCNT3_HIGH) << 8) >>>>> - | __raw_readl(info->ppmu.base + PPMU_PMCNT3_LOW)); >>>>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_HIGH, &pmcnt3_high); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_LOW, &pmcnt3_low); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + edata->load_count = ((pmcnt3_high << 8) | pmcnt3_low); >>>>> break; >>>>> default: >>>>> return -EINVAL; >>>>> } >>>>> >>>>> /* Disable specific counter */ >>>>> - cntenc = __raw_readl(info->ppmu.base + PPMU_CNTENC); >>>>> + ret = regmap_read(info->regmap, PPMU_CNTENC, &cntenc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>> - __raw_writel(cntenc, info->ppmu.base + PPMU_CNTENC); >>>>> + ret = regmap_write(info->regmap, PPMU_CNTENC, cntenc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name, >>>>> edata->load_count, edata->total_count); >>>>> @@ -214,36 +260,93 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>>>> static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> + int ret; >>>>> u32 pmnc, clear; >>>>> >>>>> /* Disable all counters */ >>>>> clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK >>>>> | PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_FLAG, clear); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_INTENC, clear); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_FLAG); >>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_INTENC); >>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNTENC); >>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNT_RESET); >>>>> - >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG0); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG1); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG2); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_RESULT); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CNT_AUTO); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV0_TYPE); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV1_TYPE); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV2_TYPE); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV3_TYPE); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_V); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_A); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_V); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_A); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_INTERRUPT_RESET); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, clear); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_RESET, clear); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG0, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG1, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG2, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_RESULT, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_AUTO, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV0_TYPE, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV1_TYPE, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV2_TYPE, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV3_TYPE, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_V, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_A, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_V, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_A, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_INTERRUPT_RESET, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Disable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> return 0; >>>>> } >>>>> @@ -251,30 +354,43 @@ static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>>>> static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> + unsigned int pmnc, cntens; >>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>> - u32 pmnc, cntens; >>>>> + int ret; >>>>> >>>>> /* Enable all counters */ >>>>> - cntens = __raw_readl(info->ppmu.base + PPMU_V2_CNTENS); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>> - __raw_writel(cntens, info->ppmu.base + PPMU_V2_CNTENS); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENS, cntens); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Set the event of Read/Write data count */ >>>>> switch (id) { >>>>> case PPMU_PMNCNT0: >>>>> case PPMU_PMNCNT1: >>>>> case PPMU_PMNCNT2: >>>>> - __raw_writel(PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT, >>>>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>>>> + PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> break; >>>>> case PPMU_PMNCNT3: >>>>> - __raw_writel(PPMU_V2_EVT3_RW_DATA_CNT, >>>>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>>>> + PPMU_V2_EVT3_RW_DATA_CNT); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> break; >>>>> } >>>>> >>>>> /* Reset cycle counter/performance counter and enable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>>>> | PPMU_PMNC_COUNTER_RESET_MASK >>>>> | PPMU_PMNC_CC_RESET_MASK >>>>> @@ -284,7 +400,10 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>>>> pmnc |= (PPMU_V2_MODE_MANUAL << PPMU_V2_PMNC_START_MODE_SHIFT); >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> return 0; >>>>> } >>>>> @@ -294,37 +413,61 @@ static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev, >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>> - u32 pmnc, cntenc; >>>>> - u32 pmcnt_high, pmcnt_low; >>>>> - u64 load_count = 0; >>>>> + int ret; >>>>> + unsigned int pmnc, cntenc; >>>>> + unsigned int pmcnt_high, pmcnt_low; >>>>> + unsigned int total_count, count; >>>>> + unsigned long load_count = 0; >>>>> >>>>> /* Disable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Read cycle count and performance count */ >>>>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_V2_CCNT); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_CCNT, &total_count); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + edata->total_count = total_count; >>>>> >>>>> switch (id) { >>>>> case PPMU_PMNCNT0: >>>>> case PPMU_PMNCNT1: >>>>> case PPMU_PMNCNT2: >>>>> - load_count = __raw_readl(info->ppmu.base + PPMU_V2_PMNCT(id)); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNCT(id), &count); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + load_count = count; >>>>> break; >>>>> case PPMU_PMNCNT3: >>>>> - pmcnt_high = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_HIGH); >>>>> - pmcnt_low = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_LOW); >>>>> - load_count = ((u64)((pmcnt_high & 0xff)) << 32) >>>>> - + (u64)pmcnt_low; >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_HIGH, >>>>> + &pmcnt_high); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_LOW, &pmcnt_low); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + load_count = ((u64)((pmcnt_high & 0xff)) << 32)+ (u64)pmcnt_low; >>>>> break; >>>>> } >>>>> edata->load_count = load_count; >>>>> >>>>> /* Disable all counters */ >>>>> - cntenc = __raw_readl(info->ppmu.base + PPMU_V2_CNTENC); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENC, &cntenc); >>>>> + if (ret < 0) >>>>> + return 0; >>>>> + >>>>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>> - __raw_writel(cntenc, info->ppmu.base + PPMU_V2_CNTENC); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, cntenc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name, >>>>> edata->load_count, edata->total_count); >>>>> @@ -411,10 +554,19 @@ static int of_get_devfreq_events(struct device_node *np, >>>>> return 0; >>>>> } >>>>> >>>>> -static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>>> +static struct regmap_config exynos_ppmu_regmap_config = { >>>>> + .reg_bits = 32, >>>>> + .val_bits = 32, >>>>> + .reg_stride = 4, >>>>> +}; >>>>> + >>>>> +static int exynos_ppmu_parse_dt(struct platform_device *pdev, >>>>> + struct exynos_ppmu *info) >>>>> { >>>>> struct device *dev = info->dev; >>>>> struct device_node *np = dev->of_node; >>>>> + struct resource *res; >>>>> + void __iomem *base; >>>>> int ret = 0; >>>>> >>>>> if (!np) { >>>>> @@ -423,10 +575,17 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>>> } >>>>> >>>>> /* Maps the memory mapped IO to control PPMU register */ >>>>> - info->ppmu.base = of_iomap(np, 0); >>>>> - if (IS_ERR_OR_NULL(info->ppmu.base)) { >>>>> - dev_err(dev, "failed to map memory region\n"); >>>>> - return -ENOMEM; >>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + base = devm_ioremap_resource(dev, res); >>>>> + if (IS_ERR(base)) >>>>> + return PTR_ERR(base); >>>>> + >>>>> + exynos_ppmu_regmap_config.max_register = resource_size(res) - 4; >>>>> + info->regmap = devm_regmap_init_mmio(dev, base, >>>>> + &exynos_ppmu_regmap_config); >>>>> + if (IS_ERR(info->regmap)) { >>>>> + dev_err(dev, "failed to initialize regmap\n"); >>>>> + return PTR_ERR(info->regmap); >>>>> } >>>>> >>>>> info->ppmu.clk = devm_clk_get(dev, "ppmu"); >>>>> @@ -438,15 +597,10 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>>> ret = of_get_devfreq_events(np, info); >>>>> if (ret < 0) { >>>>> dev_err(dev, "failed to parse exynos ppmu dt node\n"); >>>>> - goto err; >>>>> + return ret; >>>>> } >>>>> >>>>> return 0; >>>>> - >>>>> -err: >>>>> - iounmap(info->ppmu.base); >>>>> - >>>>> - return ret; >>>>> } >>>>> >>>>> static int exynos_ppmu_probe(struct platform_device *pdev) >>>>> @@ -463,7 +617,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>>> info->dev = &pdev->dev; >>>>> >>>>> /* Parse dt data to get resource */ >>>>> - ret = exynos_ppmu_parse_dt(info); >>>>> + ret = exynos_ppmu_parse_dt(pdev, info); >>>>> if (ret < 0) { >>>>> dev_err(&pdev->dev, >>>>> "failed to parse devicetree for resource\n"); >>>>> @@ -476,8 +630,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>>> if (!info->edev) { >>>>> dev_err(&pdev->dev, >>>>> "failed to allocate memory devfreq-event devices\n"); >>>>> - ret = -ENOMEM; >>>>> - goto err; >>>>> + return -ENOMEM; >>>>> } >>>>> edev = info->edev; >>>>> platform_set_drvdata(pdev, info); >>>>> @@ -488,17 +641,13 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>>> ret = PTR_ERR(edev[i]); >>>>> dev_err(&pdev->dev, >>>>> "failed to add devfreq-event device\n"); >>>>> - goto err; >>>>> + return PTR_ERR(edev[i]); >>>>> } >>>>> } >>>>> >>>>> clk_prepare_enable(info->ppmu.clk); >>>>> >>>>> return 0; >>>>> -err: >>>>> - iounmap(info->ppmu.base); >>>>> - >>>>> - return ret; >>>>> } >>>>> >>>>> static int exynos_ppmu_remove(struct platform_device *pdev) >>>>> @@ -506,7 +655,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev) >>>>> struct exynos_ppmu *info = platform_get_drvdata(pdev); >>>>> >>>>> clk_disable_unprepare(info->ppmu.clk); >>>>> - iounmap(info->ppmu.base); >>>>> >>>>> return 0; >>>>> } >>>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> >>>> >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > > -- Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-20 8:32 ` Chanwoo Choi @ 2016-12-20 13:47 ` Tobias Jakobi 0 siblings, 0 replies; 17+ messages in thread From: Tobias Jakobi @ 2016-12-20 13:47 UTC (permalink / raw) To: Chanwoo Choi, Tobias Jakobi, myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc Hey Chanwoo, Chanwoo Choi wrote: > On 2016년 12월 20일 17:26, Chanwoo Choi wrote: >> On 2016년 12월 20일 17:08, Tobias Jakobi wrote: >>> Hello Chanwoo, >>> >>> >>> Chanwoo Choi wrote: >>>> Hi, >>>> >>>> On 2016년 12월 20일 04:47, Tobias Jakobi wrote: >>>>> Hello, >>>>> >>>>> I was just wondering what is improved by moving to regmap. For me this >>>>> looks like it only complicates the code. Lots of regmap_{read,write}() >>>>> and for each one of these we need to check the return code. >>>> >>>> It is correct to check the return value. It cover all of exception. >>> that doesn't really answer my question. Which 'exceptions' are we >>> talking about? What can go wrong with __raw_{writel,readl}(), that >> >> When using __raw_readl/writel() don't check the any return value, it it not correct. >> When calling the function, basically we should check whether return value is error or success. >> What is problem to check the return value? > > I use 'exception' word means the error situations. When handling the > register of PPMU, the error might happen. So, we need to check the > return value by using the regmap interface. Again, what error do you want to handle here? For which reason could __raw_{writel,readl}() fail here, to make it necessary to go through regmap for this? With best wishes, Tobias >>> makes it necessary to put another layer on top of it? AFAIK regmap was >>> introduced to handle read/writes to slow busses like I2C and SPI. I >>> don't see that this applies here. >> >> The regmap support the MMIO on following driver. >> - drivers/base/regmap/regmap-mmio.c >> >>> >>> >>>>> Also when exactly did __raw_writel() and friends become legacy? >>>> >>>> Also, I want to use the regmap interface (regmap-io) instead of __raw_readl/__raw_writel using the base address >>>> directly. >>> I see, but why are __raw_{writel,readl}() legacy as you say? I don't see >>> this anywhere else in the kernel. >> >> If you just don't like the 'legacy' expression. I'll remove it. >> It is not any important. The key point of this patch uses the regmap interface. >> Usually, when adding new device driver, I use the regmap mmio interface >> instead of __raw_readl/writel. So, this patch changes it. >> >> Regards, >> Chanwoo Choi >> >>> >>> >>> With best wishes, >>> Tobias >>> >>> >>>> >>>> Regards, >>>> Chanwoo Choi >>>> >>>>> >>>>> With best wishes, >>>>> Tobias >>>>> >>>>> >>>>> Chanwoo Choi wrote: >>>>>> This patch uses the regmap interface to read and write the registers for exynos >>>>>> PPMU device instead of the legacy memory map functions. >>>>>> >>>>>> Cc: Kukjin Kim <kgene@kernel.org> >>>>>> Cc: Krzysztof Kozlowski <krzk@kernel.org> >>>>>> Cc: Javier Martinez Canillas <javier@osg.samsung.com> >>>>>> Cc: linux-samsung-soc@vger.kernel.org >>>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>>>>> --- >>>>>> drivers/devfreq/event/exynos-ppmu.c | 326 ++++++++++++++++++++++++++---------- >>>>>> 1 file changed, 237 insertions(+), 89 deletions(-) >>>>>> >>>>>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c >>>>>> index 107eb91a9415..fb3706faf5bd 100644 >>>>>> --- a/drivers/devfreq/event/exynos-ppmu.c >>>>>> +++ b/drivers/devfreq/event/exynos-ppmu.c >>>>>> @@ -17,13 +17,13 @@ >>>>>> #include <linux/module.h> >>>>>> #include <linux/of_address.h> >>>>>> #include <linux/platform_device.h> >>>>>> +#include <linux/regmap.h> >>>>>> #include <linux/suspend.h> >>>>>> #include <linux/devfreq-event.h> >>>>>> >>>>>> #include "exynos-ppmu.h" >>>>>> >>>>>> struct exynos_ppmu_data { >>>>>> - void __iomem *base; >>>>>> struct clk *clk; >>>>>> }; >>>>>> >>>>>> @@ -33,6 +33,7 @@ struct exynos_ppmu { >>>>>> unsigned int num_events; >>>>>> >>>>>> struct device *dev; >>>>>> + struct regmap *regmap; >>>>>> >>>>>> struct exynos_ppmu_data ppmu; >>>>>> }; >>>>>> @@ -107,20 +108,28 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev) >>>>>> static int exynos_ppmu_disable(struct devfreq_event_dev *edev) >>>>>> { >>>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>>> + int ret; >>>>>> u32 pmnc; >>>>>> >>>>>> /* Disable all counters */ >>>>>> - __raw_writel(PPMU_CCNT_MASK | >>>>>> - PPMU_PMCNT0_MASK | >>>>>> - PPMU_PMCNT1_MASK | >>>>>> - PPMU_PMCNT2_MASK | >>>>>> - PPMU_PMCNT3_MASK, >>>>>> - info->ppmu.base + PPMU_CNTENC); >>>>>> + ret = regmap_write(info->regmap, PPMU_CNTENC, >>>>>> + PPMU_CCNT_MASK | >>>>>> + PPMU_PMCNT0_MASK | >>>>>> + PPMU_PMCNT1_MASK | >>>>>> + PPMU_PMCNT2_MASK | >>>>>> + PPMU_PMCNT3_MASK); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> /* Disable PPMU */ >>>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> return 0; >>>>>> } >>>>>> @@ -129,29 +138,42 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev) >>>>>> { >>>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>>> + int ret; >>>>>> u32 pmnc, cntens; >>>>>> >>>>>> if (id < 0) >>>>>> return id; >>>>>> >>>>>> /* Enable specific counter */ >>>>>> - cntens = __raw_readl(info->ppmu.base + PPMU_CNTENS); >>>>>> + ret = regmap_read(info->regmap, PPMU_CNTENS, &cntens); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>>> - __raw_writel(cntens, info->ppmu.base + PPMU_CNTENS); >>>>>> + ret = regmap_write(info->regmap, PPMU_CNTENS, cntens); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> /* Set the event of Read/Write data count */ >>>>>> - __raw_writel(PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT, >>>>>> - info->ppmu.base + PPMU_BEVTxSEL(id)); >>>>>> + ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), >>>>>> + PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> /* Reset cycle counter/performance counter and enable PPMU */ >>>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>>>>> | PPMU_PMNC_COUNTER_RESET_MASK >>>>>> | PPMU_PMNC_CC_RESET_MASK); >>>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT); >>>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> return 0; >>>>>> } >>>>>> @@ -161,40 +183,64 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>>>>> { >>>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>>> - u32 pmnc, cntenc; >>>>>> + unsigned int total_count, load_count; >>>>>> + unsigned int pmcnt3_high, pmcnt3_low; >>>>>> + unsigned int pmnc, cntenc; >>>>>> + int ret; >>>>>> >>>>>> if (id < 0) >>>>>> return -EINVAL; >>>>>> >>>>>> /* Disable PPMU */ >>>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> /* Read cycle count */ >>>>>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_CCNT); >>>>>> + ret = regmap_read(info->regmap, PPMU_CCNT, &total_count); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + edata->total_count = total_count; >>>>>> >>>>>> /* Read performance count */ >>>>>> switch (id) { >>>>>> case PPMU_PMNCNT0: >>>>>> case PPMU_PMNCNT1: >>>>>> case PPMU_PMNCNT2: >>>>>> - edata->load_count >>>>>> - = __raw_readl(info->ppmu.base + PPMU_PMNCT(id)); >>>>>> + ret = regmap_read(info->regmap, PPMU_PMNCT(id), &load_count); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + edata->load_count = load_count; >>>>>> break; >>>>>> case PPMU_PMNCNT3: >>>>>> - edata->load_count = >>>>>> - ((__raw_readl(info->ppmu.base + PPMU_PMCNT3_HIGH) << 8) >>>>>> - | __raw_readl(info->ppmu.base + PPMU_PMCNT3_LOW)); >>>>>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_HIGH, &pmcnt3_high); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_LOW, &pmcnt3_low); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + edata->load_count = ((pmcnt3_high << 8) | pmcnt3_low); >>>>>> break; >>>>>> default: >>>>>> return -EINVAL; >>>>>> } >>>>>> >>>>>> /* Disable specific counter */ >>>>>> - cntenc = __raw_readl(info->ppmu.base + PPMU_CNTENC); >>>>>> + ret = regmap_read(info->regmap, PPMU_CNTENC, &cntenc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>>> - __raw_writel(cntenc, info->ppmu.base + PPMU_CNTENC); >>>>>> + ret = regmap_write(info->regmap, PPMU_CNTENC, cntenc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name, >>>>>> edata->load_count, edata->total_count); >>>>>> @@ -214,36 +260,93 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>>>>> static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>>>>> { >>>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>>> + int ret; >>>>>> u32 pmnc, clear; >>>>>> >>>>>> /* Disable all counters */ >>>>>> clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK >>>>>> | PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK); >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_FLAG, clear); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_INTENC, clear); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_FLAG); >>>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_INTENC); >>>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNTENC); >>>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNT_RESET); >>>>>> - >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG0); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG1); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG2); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_RESULT); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CNT_AUTO); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV0_TYPE); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV1_TYPE); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV2_TYPE); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV3_TYPE); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_V); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_A); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_V); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_A); >>>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_INTERRUPT_RESET); >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, clear); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_RESET, clear); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG0, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG1, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG2, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_RESULT, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_AUTO, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV0_TYPE, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV1_TYPE, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV2_TYPE, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV3_TYPE, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_V, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_A, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_V, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_A, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_INTERRUPT_RESET, 0x0); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> /* Disable PPMU */ >>>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> return 0; >>>>>> } >>>>>> @@ -251,30 +354,43 @@ static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>>>>> static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>>>>> { >>>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>>> + unsigned int pmnc, cntens; >>>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>>> - u32 pmnc, cntens; >>>>>> + int ret; >>>>>> >>>>>> /* Enable all counters */ >>>>>> - cntens = __raw_readl(info->ppmu.base + PPMU_V2_CNTENS); >>>>>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>>> - __raw_writel(cntens, info->ppmu.base + PPMU_V2_CNTENS); >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENS, cntens); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> /* Set the event of Read/Write data count */ >>>>>> switch (id) { >>>>>> case PPMU_PMNCNT0: >>>>>> case PPMU_PMNCNT1: >>>>>> case PPMU_PMNCNT2: >>>>>> - __raw_writel(PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT, >>>>>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>>>>> + PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> break; >>>>>> case PPMU_PMNCNT3: >>>>>> - __raw_writel(PPMU_V2_EVT3_RW_DATA_CNT, >>>>>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>>>>> + PPMU_V2_EVT3_RW_DATA_CNT); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> break; >>>>>> } >>>>>> >>>>>> /* Reset cycle counter/performance counter and enable PPMU */ >>>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>>>>> | PPMU_PMNC_COUNTER_RESET_MASK >>>>>> | PPMU_PMNC_CC_RESET_MASK >>>>>> @@ -284,7 +400,10 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>>>>> pmnc |= (PPMU_V2_MODE_MANUAL << PPMU_V2_PMNC_START_MODE_SHIFT); >>>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>>>> + >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> return 0; >>>>>> } >>>>>> @@ -294,37 +413,61 @@ static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev, >>>>>> { >>>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>>> - u32 pmnc, cntenc; >>>>>> - u32 pmcnt_high, pmcnt_low; >>>>>> - u64 load_count = 0; >>>>>> + int ret; >>>>>> + unsigned int pmnc, cntenc; >>>>>> + unsigned int pmcnt_high, pmcnt_low; >>>>>> + unsigned int total_count, count; >>>>>> + unsigned long load_count = 0; >>>>>> >>>>>> /* Disable PPMU */ >>>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> /* Read cycle count and performance count */ >>>>>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_V2_CCNT); >>>>>> + ret = regmap_read(info->regmap, PPMU_V2_CCNT, &total_count); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + edata->total_count = total_count; >>>>>> >>>>>> switch (id) { >>>>>> case PPMU_PMNCNT0: >>>>>> case PPMU_PMNCNT1: >>>>>> case PPMU_PMNCNT2: >>>>>> - load_count = __raw_readl(info->ppmu.base + PPMU_V2_PMNCT(id)); >>>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNCT(id), &count); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + load_count = count; >>>>>> break; >>>>>> case PPMU_PMNCNT3: >>>>>> - pmcnt_high = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_HIGH); >>>>>> - pmcnt_low = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_LOW); >>>>>> - load_count = ((u64)((pmcnt_high & 0xff)) << 32) >>>>>> - + (u64)pmcnt_low; >>>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_HIGH, >>>>>> + &pmcnt_high); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_LOW, &pmcnt_low); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + load_count = ((u64)((pmcnt_high & 0xff)) << 32)+ (u64)pmcnt_low; >>>>>> break; >>>>>> } >>>>>> edata->load_count = load_count; >>>>>> >>>>>> /* Disable all counters */ >>>>>> - cntenc = __raw_readl(info->ppmu.base + PPMU_V2_CNTENC); >>>>>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENC, &cntenc); >>>>>> + if (ret < 0) >>>>>> + return 0; >>>>>> + >>>>>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>>> - __raw_writel(cntenc, info->ppmu.base + PPMU_V2_CNTENC); >>>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, cntenc); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> >>>>>> dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name, >>>>>> edata->load_count, edata->total_count); >>>>>> @@ -411,10 +554,19 @@ static int of_get_devfreq_events(struct device_node *np, >>>>>> return 0; >>>>>> } >>>>>> >>>>>> -static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>>>> +static struct regmap_config exynos_ppmu_regmap_config = { >>>>>> + .reg_bits = 32, >>>>>> + .val_bits = 32, >>>>>> + .reg_stride = 4, >>>>>> +}; >>>>>> + >>>>>> +static int exynos_ppmu_parse_dt(struct platform_device *pdev, >>>>>> + struct exynos_ppmu *info) >>>>>> { >>>>>> struct device *dev = info->dev; >>>>>> struct device_node *np = dev->of_node; >>>>>> + struct resource *res; >>>>>> + void __iomem *base; >>>>>> int ret = 0; >>>>>> >>>>>> if (!np) { >>>>>> @@ -423,10 +575,17 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>>>> } >>>>>> >>>>>> /* Maps the memory mapped IO to control PPMU register */ >>>>>> - info->ppmu.base = of_iomap(np, 0); >>>>>> - if (IS_ERR_OR_NULL(info->ppmu.base)) { >>>>>> - dev_err(dev, "failed to map memory region\n"); >>>>>> - return -ENOMEM; >>>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>>> + base = devm_ioremap_resource(dev, res); >>>>>> + if (IS_ERR(base)) >>>>>> + return PTR_ERR(base); >>>>>> + >>>>>> + exynos_ppmu_regmap_config.max_register = resource_size(res) - 4; >>>>>> + info->regmap = devm_regmap_init_mmio(dev, base, >>>>>> + &exynos_ppmu_regmap_config); >>>>>> + if (IS_ERR(info->regmap)) { >>>>>> + dev_err(dev, "failed to initialize regmap\n"); >>>>>> + return PTR_ERR(info->regmap); >>>>>> } >>>>>> >>>>>> info->ppmu.clk = devm_clk_get(dev, "ppmu"); >>>>>> @@ -438,15 +597,10 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>>>> ret = of_get_devfreq_events(np, info); >>>>>> if (ret < 0) { >>>>>> dev_err(dev, "failed to parse exynos ppmu dt node\n"); >>>>>> - goto err; >>>>>> + return ret; >>>>>> } >>>>>> >>>>>> return 0; >>>>>> - >>>>>> -err: >>>>>> - iounmap(info->ppmu.base); >>>>>> - >>>>>> - return ret; >>>>>> } >>>>>> >>>>>> static int exynos_ppmu_probe(struct platform_device *pdev) >>>>>> @@ -463,7 +617,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>>>> info->dev = &pdev->dev; >>>>>> >>>>>> /* Parse dt data to get resource */ >>>>>> - ret = exynos_ppmu_parse_dt(info); >>>>>> + ret = exynos_ppmu_parse_dt(pdev, info); >>>>>> if (ret < 0) { >>>>>> dev_err(&pdev->dev, >>>>>> "failed to parse devicetree for resource\n"); >>>>>> @@ -476,8 +630,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>>>> if (!info->edev) { >>>>>> dev_err(&pdev->dev, >>>>>> "failed to allocate memory devfreq-event devices\n"); >>>>>> - ret = -ENOMEM; >>>>>> - goto err; >>>>>> + return -ENOMEM; >>>>>> } >>>>>> edev = info->edev; >>>>>> platform_set_drvdata(pdev, info); >>>>>> @@ -488,17 +641,13 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>>>> ret = PTR_ERR(edev[i]); >>>>>> dev_err(&pdev->dev, >>>>>> "failed to add devfreq-event device\n"); >>>>>> - goto err; >>>>>> + return PTR_ERR(edev[i]); >>>>>> } >>>>>> } >>>>>> >>>>>> clk_prepare_enable(info->ppmu.clk); >>>>>> >>>>>> return 0; >>>>>> -err: >>>>>> - iounmap(info->ppmu.base); >>>>>> - >>>>>> - return ret; >>>>>> } >>>>>> >>>>>> static int exynos_ppmu_remove(struct platform_device *pdev) >>>>>> @@ -506,7 +655,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev) >>>>>> struct exynos_ppmu *info = platform_get_drvdata(pdev); >>>>>> >>>>>> clk_disable_unprepare(info->ppmu.clk); >>>>>> - iounmap(info->ppmu.base); >>>>>> >>>>>> return 0; >>>>>> } >>>>>> >>>>> >>>>> -- >>>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >>>>> the body of a message to majordomo@vger.kernel.org >>>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>>> >>>>> >>>>> >>>> >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> >>> >> >> > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-20 8:26 ` Chanwoo Choi 2016-12-20 8:32 ` Chanwoo Choi @ 2016-12-20 13:47 ` Tobias Jakobi 2016-12-20 19:20 ` Chanwoo Choi 1 sibling, 1 reply; 17+ messages in thread From: Tobias Jakobi @ 2016-12-20 13:47 UTC (permalink / raw) To: Chanwoo Choi, Tobias Jakobi, myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc Hey Chanwoo, Chanwoo Choi wrote: > On 2016년 12월 20일 17:08, Tobias Jakobi wrote: >> Hello Chanwoo, >> >> >> Chanwoo Choi wrote: >>> Hi, >>> >>> On 2016년 12월 20일 04:47, Tobias Jakobi wrote: >>>> Hello, >>>> >>>> I was just wondering what is improved by moving to regmap. For me this >>>> looks like it only complicates the code. Lots of regmap_{read,write}() >>>> and for each one of these we need to check the return code. >>> >>> It is correct to check the return value. It cover all of exception. >> that doesn't really answer my question. Which 'exceptions' are we >> talking about? What can go wrong with __raw_{writel,readl}(), that > > When using __raw_readl/writel() don't check the any return value, it it not correct. > When calling the function, basically we should check whether return value is error or success. > What is problem to check the return value? So what you're saying is the following. When using __raw_{readl,writel}() somde error can occur, that we can't catch by using __raw_{readl,writel}(), but only by using the regmap API on top. So, what error would that be? Do you have an example where such an error occurs? In particular this leads me to the following question: What bug does the conversion to regmap actually fix? >> makes it necessary to put another layer on top of it? AFAIK regmap was >> introduced to handle read/writes to slow busses like I2C and SPI. I >> don't see that this applies here. > > The regmap support the MMIO on following driver. > - drivers/base/regmap/regmap-mmio.c I know, but just because something exist isn't enough reason for me to using it. There should be a benefit here. At the moment I only see that this does the following: - makes the code more convoluted - does some dubious error checking - impact of performance (__raw_{readl,writel}() maps to some load/stores on the assembler level, now we have go through a whole subsystem to achieve the same thing) >>>> Also when exactly did __raw_writel() and friends become legacy? >>> >>> Also, I want to use the regmap interface (regmap-io) instead of __raw_readl/__raw_writel using the base address >>> directly. >> I see, but why are __raw_{writel,readl}() legacy as you say? I don't see >> this anywhere else in the kernel. > > If you just don't like the 'legacy' expression. I'll remove it. No, actually the 'legacy' part is important, if it were true. If __raw_{writel,readl}() would indeed be legacy and there was a consensus that using a different interface is better, then I would agree to this change. But the calls are not legacy, hence I'm missing some reason for this change. > It is not any important. The key point of this patch uses the regmap interface. > Usually, when adding new device driver, I use the regmap mmio interface > instead of __raw_readl/writel. So, this patch changes it. That doesn't sound like a good reasoning. What improvement do we get by this change? And no, I don't buy the error checking argument ;) With best wishes, Tobias > > Regards, > Chanwoo Choi > >> >> >> With best wishes, >> Tobias >> >> >>> >>> Regards, >>> Chanwoo Choi >>> >>>> >>>> With best wishes, >>>> Tobias >>>> >>>> >>>> Chanwoo Choi wrote: >>>>> This patch uses the regmap interface to read and write the registers for exynos >>>>> PPMU device instead of the legacy memory map functions. >>>>> >>>>> Cc: Kukjin Kim <kgene@kernel.org> >>>>> Cc: Krzysztof Kozlowski <krzk@kernel.org> >>>>> Cc: Javier Martinez Canillas <javier@osg.samsung.com> >>>>> Cc: linux-samsung-soc@vger.kernel.org >>>>> Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> >>>>> --- >>>>> drivers/devfreq/event/exynos-ppmu.c | 326 ++++++++++++++++++++++++++---------- >>>>> 1 file changed, 237 insertions(+), 89 deletions(-) >>>>> >>>>> diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c >>>>> index 107eb91a9415..fb3706faf5bd 100644 >>>>> --- a/drivers/devfreq/event/exynos-ppmu.c >>>>> +++ b/drivers/devfreq/event/exynos-ppmu.c >>>>> @@ -17,13 +17,13 @@ >>>>> #include <linux/module.h> >>>>> #include <linux/of_address.h> >>>>> #include <linux/platform_device.h> >>>>> +#include <linux/regmap.h> >>>>> #include <linux/suspend.h> >>>>> #include <linux/devfreq-event.h> >>>>> >>>>> #include "exynos-ppmu.h" >>>>> >>>>> struct exynos_ppmu_data { >>>>> - void __iomem *base; >>>>> struct clk *clk; >>>>> }; >>>>> >>>>> @@ -33,6 +33,7 @@ struct exynos_ppmu { >>>>> unsigned int num_events; >>>>> >>>>> struct device *dev; >>>>> + struct regmap *regmap; >>>>> >>>>> struct exynos_ppmu_data ppmu; >>>>> }; >>>>> @@ -107,20 +108,28 @@ static int exynos_ppmu_find_ppmu_id(struct devfreq_event_dev *edev) >>>>> static int exynos_ppmu_disable(struct devfreq_event_dev *edev) >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> + int ret; >>>>> u32 pmnc; >>>>> >>>>> /* Disable all counters */ >>>>> - __raw_writel(PPMU_CCNT_MASK | >>>>> - PPMU_PMCNT0_MASK | >>>>> - PPMU_PMCNT1_MASK | >>>>> - PPMU_PMCNT2_MASK | >>>>> - PPMU_PMCNT3_MASK, >>>>> - info->ppmu.base + PPMU_CNTENC); >>>>> + ret = regmap_write(info->regmap, PPMU_CNTENC, >>>>> + PPMU_CCNT_MASK | >>>>> + PPMU_PMCNT0_MASK | >>>>> + PPMU_PMCNT1_MASK | >>>>> + PPMU_PMCNT2_MASK | >>>>> + PPMU_PMCNT3_MASK); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Disable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> return 0; >>>>> } >>>>> @@ -129,29 +138,42 @@ static int exynos_ppmu_set_event(struct devfreq_event_dev *edev) >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>> + int ret; >>>>> u32 pmnc, cntens; >>>>> >>>>> if (id < 0) >>>>> return id; >>>>> >>>>> /* Enable specific counter */ >>>>> - cntens = __raw_readl(info->ppmu.base + PPMU_CNTENS); >>>>> + ret = regmap_read(info->regmap, PPMU_CNTENS, &cntens); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>> - __raw_writel(cntens, info->ppmu.base + PPMU_CNTENS); >>>>> + ret = regmap_write(info->regmap, PPMU_CNTENS, cntens); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Set the event of Read/Write data count */ >>>>> - __raw_writel(PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT, >>>>> - info->ppmu.base + PPMU_BEVTxSEL(id)); >>>>> + ret = regmap_write(info->regmap, PPMU_BEVTxSEL(id), >>>>> + PPMU_RO_DATA_CNT | PPMU_WO_DATA_CNT); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Reset cycle counter/performance counter and enable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>>>> | PPMU_PMNC_COUNTER_RESET_MASK >>>>> | PPMU_PMNC_CC_RESET_MASK); >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_ENABLE_SHIFT); >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> return 0; >>>>> } >>>>> @@ -161,40 +183,64 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>> - u32 pmnc, cntenc; >>>>> + unsigned int total_count, load_count; >>>>> + unsigned int pmcnt3_high, pmcnt3_low; >>>>> + unsigned int pmnc, cntenc; >>>>> + int ret; >>>>> >>>>> if (id < 0) >>>>> return -EINVAL; >>>>> >>>>> /* Disable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Read cycle count */ >>>>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_CCNT); >>>>> + ret = regmap_read(info->regmap, PPMU_CCNT, &total_count); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + edata->total_count = total_count; >>>>> >>>>> /* Read performance count */ >>>>> switch (id) { >>>>> case PPMU_PMNCNT0: >>>>> case PPMU_PMNCNT1: >>>>> case PPMU_PMNCNT2: >>>>> - edata->load_count >>>>> - = __raw_readl(info->ppmu.base + PPMU_PMNCT(id)); >>>>> + ret = regmap_read(info->regmap, PPMU_PMNCT(id), &load_count); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + edata->load_count = load_count; >>>>> break; >>>>> case PPMU_PMNCNT3: >>>>> - edata->load_count = >>>>> - ((__raw_readl(info->ppmu.base + PPMU_PMCNT3_HIGH) << 8) >>>>> - | __raw_readl(info->ppmu.base + PPMU_PMCNT3_LOW)); >>>>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_HIGH, &pmcnt3_high); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_read(info->regmap, PPMU_PMCNT3_LOW, &pmcnt3_low); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + edata->load_count = ((pmcnt3_high << 8) | pmcnt3_low); >>>>> break; >>>>> default: >>>>> return -EINVAL; >>>>> } >>>>> >>>>> /* Disable specific counter */ >>>>> - cntenc = __raw_readl(info->ppmu.base + PPMU_CNTENC); >>>>> + ret = regmap_read(info->regmap, PPMU_CNTENC, &cntenc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>> - __raw_writel(cntenc, info->ppmu.base + PPMU_CNTENC); >>>>> + ret = regmap_write(info->regmap, PPMU_CNTENC, cntenc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> dev_dbg(&edev->dev, "%s (event: %ld/%ld)\n", edev->desc->name, >>>>> edata->load_count, edata->total_count); >>>>> @@ -214,36 +260,93 @@ static int exynos_ppmu_get_event(struct devfreq_event_dev *edev, >>>>> static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> + int ret; >>>>> u32 pmnc, clear; >>>>> >>>>> /* Disable all counters */ >>>>> clear = (PPMU_CCNT_MASK | PPMU_PMCNT0_MASK | PPMU_PMCNT1_MASK >>>>> | PPMU_PMCNT2_MASK | PPMU_PMCNT3_MASK); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_FLAG, clear); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_INTENC, clear); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_FLAG); >>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_INTENC); >>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNTENC); >>>>> - __raw_writel(clear, info->ppmu.base + PPMU_V2_CNT_RESET); >>>>> - >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG0); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG1); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_CFG2); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CIG_RESULT); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CNT_AUTO); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV0_TYPE); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV1_TYPE); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV2_TYPE); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_CH_EV3_TYPE); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_V); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_ID_A); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_V); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_SM_OTHERS_A); >>>>> - __raw_writel(0x0, info->ppmu.base + PPMU_V2_INTERRUPT_RESET); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, clear); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_RESET, clear); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG0, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG1, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_CFG2, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CIG_RESULT, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNT_AUTO, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV0_TYPE, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV1_TYPE, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV2_TYPE, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EV3_TYPE, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_V, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_ID_A, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_V, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_SM_OTHERS_A, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_INTERRUPT_RESET, 0x0); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Disable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> return 0; >>>>> } >>>>> @@ -251,30 +354,43 @@ static int exynos_ppmu_v2_disable(struct devfreq_event_dev *edev) >>>>> static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> + unsigned int pmnc, cntens; >>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>> - u32 pmnc, cntens; >>>>> + int ret; >>>>> >>>>> /* Enable all counters */ >>>>> - cntens = __raw_readl(info->ppmu.base + PPMU_V2_CNTENS); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENS, &cntens); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> cntens |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>> - __raw_writel(cntens, info->ppmu.base + PPMU_V2_CNTENS); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENS, cntens); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Set the event of Read/Write data count */ >>>>> switch (id) { >>>>> case PPMU_PMNCNT0: >>>>> case PPMU_PMNCNT1: >>>>> case PPMU_PMNCNT2: >>>>> - __raw_writel(PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT, >>>>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>>>> + PPMU_V2_RO_DATA_CNT | PPMU_V2_WO_DATA_CNT); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> break; >>>>> case PPMU_PMNCNT3: >>>>> - __raw_writel(PPMU_V2_EVT3_RW_DATA_CNT, >>>>> - info->ppmu.base + PPMU_V2_CH_EVx_TYPE(id)); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CH_EVx_TYPE(id), >>>>> + PPMU_V2_EVT3_RW_DATA_CNT); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> break; >>>>> } >>>>> >>>>> /* Reset cycle counter/performance counter and enable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~(PPMU_PMNC_ENABLE_MASK >>>>> | PPMU_PMNC_COUNTER_RESET_MASK >>>>> | PPMU_PMNC_CC_RESET_MASK >>>>> @@ -284,7 +400,10 @@ static int exynos_ppmu_v2_set_event(struct devfreq_event_dev *edev) >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_COUNTER_RESET_SHIFT); >>>>> pmnc |= (PPMU_ENABLE << PPMU_PMNC_CC_RESET_SHIFT); >>>>> pmnc |= (PPMU_V2_MODE_MANUAL << PPMU_V2_PMNC_START_MODE_SHIFT); >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>>> + >>>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> return 0; >>>>> } >>>>> @@ -294,37 +413,61 @@ static int exynos_ppmu_v2_get_event(struct devfreq_event_dev *edev, >>>>> { >>>>> struct exynos_ppmu *info = devfreq_event_get_drvdata(edev); >>>>> int id = exynos_ppmu_find_ppmu_id(edev); >>>>> - u32 pmnc, cntenc; >>>>> - u32 pmcnt_high, pmcnt_low; >>>>> - u64 load_count = 0; >>>>> + int ret; >>>>> + unsigned int pmnc, cntenc; >>>>> + unsigned int pmcnt_high, pmcnt_low; >>>>> + unsigned int total_count, count; >>>>> + unsigned long load_count = 0; >>>>> >>>>> /* Disable PPMU */ >>>>> - pmnc = __raw_readl(info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNC, &pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> pmnc &= ~PPMU_PMNC_ENABLE_MASK; >>>>> - __raw_writel(pmnc, info->ppmu.base + PPMU_V2_PMNC); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_PMNC, pmnc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> /* Read cycle count and performance count */ >>>>> - edata->total_count = __raw_readl(info->ppmu.base + PPMU_V2_CCNT); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_CCNT, &total_count); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + edata->total_count = total_count; >>>>> >>>>> switch (id) { >>>>> case PPMU_PMNCNT0: >>>>> case PPMU_PMNCNT1: >>>>> case PPMU_PMNCNT2: >>>>> - load_count = __raw_readl(info->ppmu.base + PPMU_V2_PMNCT(id)); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMNCT(id), &count); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + load_count = count; >>>>> break; >>>>> case PPMU_PMNCNT3: >>>>> - pmcnt_high = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_HIGH); >>>>> - pmcnt_low = __raw_readl(info->ppmu.base + PPMU_V2_PMCNT3_LOW); >>>>> - load_count = ((u64)((pmcnt_high & 0xff)) << 32) >>>>> - + (u64)pmcnt_low; >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_HIGH, >>>>> + &pmcnt_high); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + ret = regmap_read(info->regmap, PPMU_V2_PMCNT3_LOW, &pmcnt_low); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + load_count = ((u64)((pmcnt_high & 0xff)) << 32)+ (u64)pmcnt_low; >>>>> break; >>>>> } >>>>> edata->load_count = load_count; >>>>> >>>>> /* Disable all counters */ >>>>> - cntenc = __raw_readl(info->ppmu.base + PPMU_V2_CNTENC); >>>>> + ret = regmap_read(info->regmap, PPMU_V2_CNTENC, &cntenc); >>>>> + if (ret < 0) >>>>> + return 0; >>>>> + >>>>> cntenc |= (PPMU_CCNT_MASK | (PPMU_ENABLE << id)); >>>>> - __raw_writel(cntenc, info->ppmu.base + PPMU_V2_CNTENC); >>>>> + ret = regmap_write(info->regmap, PPMU_V2_CNTENC, cntenc); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> >>>>> dev_dbg(&edev->dev, "%25s (load: %ld / %ld)\n", edev->desc->name, >>>>> edata->load_count, edata->total_count); >>>>> @@ -411,10 +554,19 @@ static int of_get_devfreq_events(struct device_node *np, >>>>> return 0; >>>>> } >>>>> >>>>> -static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>>> +static struct regmap_config exynos_ppmu_regmap_config = { >>>>> + .reg_bits = 32, >>>>> + .val_bits = 32, >>>>> + .reg_stride = 4, >>>>> +}; >>>>> + >>>>> +static int exynos_ppmu_parse_dt(struct platform_device *pdev, >>>>> + struct exynos_ppmu *info) >>>>> { >>>>> struct device *dev = info->dev; >>>>> struct device_node *np = dev->of_node; >>>>> + struct resource *res; >>>>> + void __iomem *base; >>>>> int ret = 0; >>>>> >>>>> if (!np) { >>>>> @@ -423,10 +575,17 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>>> } >>>>> >>>>> /* Maps the memory mapped IO to control PPMU register */ >>>>> - info->ppmu.base = of_iomap(np, 0); >>>>> - if (IS_ERR_OR_NULL(info->ppmu.base)) { >>>>> - dev_err(dev, "failed to map memory region\n"); >>>>> - return -ENOMEM; >>>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >>>>> + base = devm_ioremap_resource(dev, res); >>>>> + if (IS_ERR(base)) >>>>> + return PTR_ERR(base); >>>>> + >>>>> + exynos_ppmu_regmap_config.max_register = resource_size(res) - 4; >>>>> + info->regmap = devm_regmap_init_mmio(dev, base, >>>>> + &exynos_ppmu_regmap_config); >>>>> + if (IS_ERR(info->regmap)) { >>>>> + dev_err(dev, "failed to initialize regmap\n"); >>>>> + return PTR_ERR(info->regmap); >>>>> } >>>>> >>>>> info->ppmu.clk = devm_clk_get(dev, "ppmu"); >>>>> @@ -438,15 +597,10 @@ static int exynos_ppmu_parse_dt(struct exynos_ppmu *info) >>>>> ret = of_get_devfreq_events(np, info); >>>>> if (ret < 0) { >>>>> dev_err(dev, "failed to parse exynos ppmu dt node\n"); >>>>> - goto err; >>>>> + return ret; >>>>> } >>>>> >>>>> return 0; >>>>> - >>>>> -err: >>>>> - iounmap(info->ppmu.base); >>>>> - >>>>> - return ret; >>>>> } >>>>> >>>>> static int exynos_ppmu_probe(struct platform_device *pdev) >>>>> @@ -463,7 +617,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>>> info->dev = &pdev->dev; >>>>> >>>>> /* Parse dt data to get resource */ >>>>> - ret = exynos_ppmu_parse_dt(info); >>>>> + ret = exynos_ppmu_parse_dt(pdev, info); >>>>> if (ret < 0) { >>>>> dev_err(&pdev->dev, >>>>> "failed to parse devicetree for resource\n"); >>>>> @@ -476,8 +630,7 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>>> if (!info->edev) { >>>>> dev_err(&pdev->dev, >>>>> "failed to allocate memory devfreq-event devices\n"); >>>>> - ret = -ENOMEM; >>>>> - goto err; >>>>> + return -ENOMEM; >>>>> } >>>>> edev = info->edev; >>>>> platform_set_drvdata(pdev, info); >>>>> @@ -488,17 +641,13 @@ static int exynos_ppmu_probe(struct platform_device *pdev) >>>>> ret = PTR_ERR(edev[i]); >>>>> dev_err(&pdev->dev, >>>>> "failed to add devfreq-event device\n"); >>>>> - goto err; >>>>> + return PTR_ERR(edev[i]); >>>>> } >>>>> } >>>>> >>>>> clk_prepare_enable(info->ppmu.clk); >>>>> >>>>> return 0; >>>>> -err: >>>>> - iounmap(info->ppmu.base); >>>>> - >>>>> - return ret; >>>>> } >>>>> >>>>> static int exynos_ppmu_remove(struct platform_device *pdev) >>>>> @@ -506,7 +655,6 @@ static int exynos_ppmu_remove(struct platform_device *pdev) >>>>> struct exynos_ppmu *info = platform_get_drvdata(pdev); >>>>> >>>>> clk_disable_unprepare(info->ppmu.clk); >>>>> - iounmap(info->ppmu.base); >>>>> >>>>> return 0; >>>>> } >>>>> >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >>>> the body of a message to majordomo@vger.kernel.org >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>>> >>>> >>>> >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-pm" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-20 13:47 ` Tobias Jakobi @ 2016-12-20 19:20 ` Chanwoo Choi 2017-01-05 16:07 ` Tobias Jakobi 0 siblings, 1 reply; 17+ messages in thread From: Chanwoo Choi @ 2016-12-20 19:20 UTC (permalink / raw) To: Tobias Jakobi Cc: Chanwoo Choi, myungjoo.ham@samsung.com, Kyungmin Park, Rafael J. Wysocki, linux-pm@vger.kernel.org, linux-kernel, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc 2016-12-20 22:47 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>: > Hey Chanwoo, > > > Chanwoo Choi wrote: >> On 2016년 12월 20일 17:08, Tobias Jakobi wrote: >>> Hello Chanwoo, >>> >>> >>> Chanwoo Choi wrote: >>>> Hi, >>>> >>>> On 2016년 12월 20일 04:47, Tobias Jakobi wrote: >>>>> Hello, >>>>> >>>>> I was just wondering what is improved by moving to regmap. For me this >>>>> looks like it only complicates the code. Lots of regmap_{read,write}() >>>>> and for each one of these we need to check the return code. >>>> >>>> It is correct to check the return value. It cover all of exception. >>> that doesn't really answer my question. Which 'exceptions' are we >>> talking about? What can go wrong with __raw_{writel,readl}(), that >> >> When using __raw_readl/writel() don't check the any return value, it it not correct. >> When calling the function, basically we should check whether return value is error or success. >> What is problem to check the return value? > So what you're saying is the following. When using > __raw_{readl,writel}() somde error can occur, that we can't catch by > using __raw_{readl,writel}(), but only by using the regmap API on top. > > So, what error would that be? Do you have an example where such an error > occurs? In particular this leads me to the following question: What bug > does the conversion to regmap actually fix? I don't mention that this patch is bug fix. No. It is well working. There is no any know error. As I already said, First is checking the return value of function call as following. >> When calling the function, basically we should check whether return value is error or success. > > >>> makes it necessary to put another layer on top of it? AFAIK regmap was >>> introduced to handle read/writes to slow busses like I2C and SPI. I >>> don't see that this applies here. >> >> The regmap support the MMIO on following driver. >> - drivers/base/regmap/regmap-mmio.c > I know, but just because something exist isn't enough reason for me to > using it. There should be a benefit here. > > At the moment I only see that this does the following: > - makes the code more convoluted I don't agree. As I already said as following, first is checking the return value of function call as following. ">> When calling the function, basically we should check whether return value is error or success." > - does some dubious error checking I don't want you use 'dobious' word. I need correct reason why you do obeject to use it. The error checking is clear. > - impact of performance (__raw_{readl,writel}() maps to some load/stores > on the assembler level, now we have go through a whole subsystem to > achieve the same thing) Do you have the performance result between regmap and __raw_readl/writel? Do you mean that regmap-mmio is unneeded? It is not reasonable. The system is enough fast to use the regmap. The many device driver use the 'regmap-mmio.c' driver in mainline kernel. You can find them. > > >>>>> Also when exactly did __raw_writel() and friends become legacy? >>>> >>>> Also, I want to use the regmap interface (regmap-io) instead of __raw_readl/__raw_writel using the base address >>>> directly. >>> I see, but why are __raw_{writel,readl}() legacy as you say? I don't see >>> this anywhere else in the kernel. >> >> If you just don't like the 'legacy' expression. I'll remove it. > No, actually the 'legacy' part is important, if it were true. If > __raw_{writel,readl}() would indeed be legacy and there was a consensus > that using a different interface is better, then I would agree to this > change. > But the calls are not legacy, hence I'm missing some reason for this change. When using devm_regmap_init_mmio(), the device driver don't need to consider the 'iounmap' becaue it is automatically by framework. And when using __raw_readl/writel to read and write the registers, they check whether register is writable/readable is not. The __raw_readl/writel don't consider them and support them. > > >> It is not any important. The key point of this patch uses the regmap interface. >> Usually, when adding new device driver, I use the regmap mmio interface >> instead of __raw_readl/writel. So, this patch changes it. > That doesn't sound like a good reasoning. What improvement do we get by > this change? And no, I don't buy the error checking argument ;) I replied already why regmap interface is used. -- Best Regards, Chanwoo Choi ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers 2016-12-20 19:20 ` Chanwoo Choi @ 2017-01-05 16:07 ` Tobias Jakobi 0 siblings, 0 replies; 17+ messages in thread From: Tobias Jakobi @ 2017-01-05 16:07 UTC (permalink / raw) To: cwchoi00 Cc: Chanwoo Choi, myungjoo.ham@samsung.com, Kyungmin Park, Rafael J. Wysocki, linux-pm@vger.kernel.org, linux-kernel, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc Hello Chanwoo, sorry for the late reply. I was staying with my parents over the Christmas days and didn't have access to the board. Also internet connectivity was a bit troublesome *rolleyes* Chanwoo Choi wrote: > 2016-12-20 22:47 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>: >> Hey Chanwoo, >> >> >> Chanwoo Choi wrote: >>> On 2016년 12월 20일 17:08, Tobias Jakobi wrote: >>>> Hello Chanwoo, >>>> >>>> >>>> Chanwoo Choi wrote: >>>>> Hi, >>>>> >>>>> On 2016년 12월 20일 04:47, Tobias Jakobi wrote: >>>>>> Hello, >>>>>> >>>>>> I was just wondering what is improved by moving to regmap. For me this >>>>>> looks like it only complicates the code. Lots of regmap_{read,write}() >>>>>> and for each one of these we need to check the return code. >>>>> >>>>> It is correct to check the return value. It cover all of exception. >>>> that doesn't really answer my question. Which 'exceptions' are we >>>> talking about? What can go wrong with __raw_{writel,readl}(), that >>> >>> When using __raw_readl/writel() don't check the any return value, it it not correct. >>> When calling the function, basically we should check whether return value is error or success. >>> What is problem to check the return value? >> So what you're saying is the following. When using >> __raw_{readl,writel}() somde error can occur, that we can't catch by >> using __raw_{readl,writel}(), but only by using the regmap API on top. > >> >> So, what error would that be? Do you have an example where such an error >> occurs? In particular this leads me to the following question: What bug >> does the conversion to regmap actually fix? > > I don't mention that this patch is bug fix. > > No. It is well working. There is no any know error. As I already said, > First is checking the return value of function call as following. >>> When calling the function, basically we should check whether return value is error or success. I've spend some time to go through the regmap core and the regmap mmio code, and I don't see the point in this conversion. What you do is to replace __raw_{readl,writel}() with writel()/readl(), which is what regmap mmio calls internally. So in particular this adds memory barriers. Plus all the indirection, spinlock locking/unlocking, etc. See below for a detailed analysis of regmap_write(). >>>> makes it necessary to put another layer on top of it? AFAIK regmap was >>>> introduced to handle read/writes to slow busses like I2C and SPI. I >>>> don't see that this applies here. >>> >>> The regmap support the MMIO on following driver. >>> - drivers/base/regmap/regmap-mmio.c >> I know, but just because something exist isn't enough reason for me to >> using it. There should be a benefit here. >> >> At the moment I only see that this does the following: >> - makes the code more convoluted > > I don't agree. As I already said as following, first is checking the > return value of function call as following. > ">> When calling the function, basically we should check whether > return value is error or success." You want to have some error code returned just for the sake of having an error code. I don't follow this reasoning. In particular if it involves putting a indirection layer on top, without gaining anything from it. The conversion doesn't make the code cleaner, which can be already seen from the diffstat (much more lines added than removes). It also doesn't make the code safer. >> - does some dubious error checking > > I don't want you use 'dobious' word. I need correct reason why you do > obeject to use it. No offense meant by 'dubious', but it is indeed the case here. :) > The error checking is clear. No, it's not clear to me. Please see the regmap core code yourself, and ask yourself what kind of errors you're actually checking here. Take e.g. regmap_write() and trace the return statements. - alignment check [in regmap_write] We already know that we always have correct alignment. - writable check [in _regmap_write] We already know that the code only writes to writable registers. - clock checks [in regmap_mmio_write] We don't do clock management via regmap. If you look at regmap_mmio_write() again, you see that no error value is propagated from ctx->reg_write(ctx, reg, val) at all. If you omit the clock stuff, then regmap_mmio_write() always returns zero. Hence we have replaced a simple __raw_writel(), a function that maps to a single (!) CPU instruction on ARM, with a bunch of checks that are known (by static analysis) to never fail, plus the write instruction with memory barriers. And we do all this _every_ _single_ _time_. For a particular bad example see the zero-initialisation in exynos_ppmu_v2_disable(). Sorry, but by all means, this is not a improvement of the code. > >> - impact of performance (__raw_{readl,writel}() maps to some load/stores >> on the assembler level, now we have go through a whole subsystem to >> achieve the same thing) > > Do you have the performance result between regmap and __raw_readl/writel? My preliminary tests show a increase of CPU cycles spend to access these registers by several orders of magnitude. Not surprising, with the memory barriers and the spinlocks now in place. Maybe I have some time on the weekend to do some further tests. > Do you mean that regmap-mmio is unneeded? I'm saying that in _this_ particular case, the use of regmap-mmio is counterproductive. > It is not reasonable. The system is enough fast to use the regmap. The > many device driver use the 'regmap-mmio.c' driver in mainline kernel. > You can find them. Sure, you could probably add six more abstraction layers on top and the system would still be fast enough to handle ut, but I don't see why you want to make code slower on _purpose_, just for the sake of using regmap-mmio. regmap-mmio has its purpose, but I disagree that it applies here (both for exynos-bus and ppmu). The diffstat again supports my point. > >> >> >>>>>> Also when exactly did __raw_writel() and friends become legacy? >>>>> >>>>> Also, I want to use the regmap interface (regmap-io) instead of __raw_readl/__raw_writel using the base address >>>>> directly. >>>> I see, but why are __raw_{writel,readl}() legacy as you say? I don't see >>>> this anywhere else in the kernel. >>> >>> If you just don't like the 'legacy' expression. I'll remove it. >> No, actually the 'legacy' part is important, if it were true. If >> __raw_{writel,readl}() would indeed be legacy and there was a consensus >> that using a different interface is better, then I would agree to this >> change. >> But the calls are not legacy, hence I'm missing some reason for this change. > > When using devm_regmap_init_mmio(), the device driver don't need to > consider the 'iounmap' becaue it is automatically by framework. And > when using __raw_readl/writel to read and write the registers, they > check whether register is writable/readable is not. This is no improvement. First of all the amount of init code in exynos_ppmu_parse_dt() has increased with your patch. Instead of of_iomap() you now have platform_get_resource(), devm_ioremap_resource() and devm_regmap_init_mmio() there. If iounmap() really were a problem, which I doubt it is, one could just use devm_ioremap(). The writable/readable argument is also none, since you have to supply this information yourself/explicitly. We know beforehand which registers are of which type. This is not a situation where e.g. userspace passes us some values, which might potentially be wrong. We know when a register is writable and when it's not. Everything else would a bug in the code itself. > > The __raw_readl/writel don't consider them and support them. > >> >> >>> It is not any important. The key point of this patch uses the regmap interface. >>> Usually, when adding new device driver, I use the regmap mmio interface >>> instead of __raw_readl/writel. So, this patch changes it. >> That doesn't sound like a good reasoning. What improvement do we get by >> this change? And no, I don't buy the error checking argument ;) > > I replied already why regmap interface is used. > With best wishes, Tobias ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/7] PM / devfreq: exynos-bus: Print the real clock rate of bus 2016-12-15 9:30 [PATCH 0/7] PM / devfreq: Update the devfreq and devfreq-event device Chanwoo Choi 2016-12-15 9:30 ` [PATCH 1/7] PM / devfreq: exynos-bus: Add the detailed correlation for Exynos5433 Chanwoo Choi 2016-12-15 9:30 ` [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers Chanwoo Choi @ 2016-12-15 9:30 ` Chanwoo Choi 2016-12-15 9:30 ` [PATCH 4/7] PM / devfreq: exynos-ppmu: Show the registred device for ppmu device Chanwoo Choi ` (3 subsequent siblings) 6 siblings, 0 replies; 17+ messages in thread From: Chanwoo Choi @ 2016-12-15 9:30 UTC (permalink / raw) To: myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Chanwoo Choi, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc This patch shows the real clock rate after calling clk_set_rate() to debug it. Cc: Kukjin Kim <kgene@kernel.org> Cc: Krzysztof Kozlowski <krzk@kernel.org> Cc: Javier Martinez Canillas <javier@osg.samsung.com> Cc: linux-samsung-soc@vger.kernel.org Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/devfreq/exynos-bus.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c index 9af86f46fbec..e0d1f4ac1740 100644 --- a/drivers/devfreq/exynos-bus.c +++ b/drivers/devfreq/exynos-bus.c @@ -147,8 +147,8 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags) } bus->curr_freq = new_freq; - dev_dbg(dev, "Set the frequency of bus (%lukHz -> %lukHz)\n", - old_freq/1000, new_freq/1000); + dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n", + old_freq, new_freq, clk_get_rate(bus->clk)); out: mutex_unlock(&bus->lock); @@ -241,8 +241,8 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq, *freq = new_freq; bus->curr_freq = new_freq; - dev_dbg(dev, "Set the frequency of bus (%lukHz -> %lukHz)\n", - old_freq/1000, new_freq/1000); + dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n", + old_freq, new_freq, clk_get_rate(bus->clk)); out: mutex_unlock(&bus->lock); -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/7] PM / devfreq: exynos-ppmu: Show the registred device for ppmu device 2016-12-15 9:30 [PATCH 0/7] PM / devfreq: Update the devfreq and devfreq-event device Chanwoo Choi ` (2 preceding siblings ...) 2016-12-15 9:30 ` [PATCH 3/7] PM / devfreq: exynos-bus: Print the real clock rate of bus Chanwoo Choi @ 2016-12-15 9:30 ` Chanwoo Choi 2016-12-15 9:30 ` [PATCH 5/7] PM / devfreq: Fix the checkpatch warnings Chanwoo Choi ` (2 subsequent siblings) 6 siblings, 0 replies; 17+ messages in thread From: Chanwoo Choi @ 2016-12-15 9:30 UTC (permalink / raw) To: myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Chanwoo Choi, Kukjin Kim, Krzysztof Kozlowski, Javier Martinez Canillas, linux-samsung-soc This patch just adds the simple log to show the PPMU device's registration during the kernel booting. Cc: Kukjin Kim <kgene@kernel.org> Cc: Krzysztof Kozlowski <krzk@kernel.org> Cc: Javier Martinez Canillas <javier@osg.samsung.com> Cc: linux-samsung-soc@vger.kernel.org Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/devfreq/event/exynos-ppmu.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/devfreq/event/exynos-ppmu.c b/drivers/devfreq/event/exynos-ppmu.c index fb3706faf5bd..9b5294d0bff4 100644 --- a/drivers/devfreq/event/exynos-ppmu.c +++ b/drivers/devfreq/event/exynos-ppmu.c @@ -591,7 +591,7 @@ static int exynos_ppmu_parse_dt(struct platform_device *pdev, info->ppmu.clk = devm_clk_get(dev, "ppmu"); if (IS_ERR(info->ppmu.clk)) { info->ppmu.clk = NULL; - dev_warn(dev, "cannot get PPMU clock\n"); + dev_dbg(dev, "cannot get PPMU clock\n"); } ret = of_get_devfreq_events(np, info); @@ -643,6 +643,9 @@ static int exynos_ppmu_probe(struct platform_device *pdev) "failed to add devfreq-event device\n"); return PTR_ERR(edev[i]); } + + pr_info("exynos-ppmu: new PPMU device registered %s (%s)\n", + dev_name(&pdev->dev), desc[i].name); } clk_prepare_enable(info->ppmu.clk); -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 5/7] PM / devfreq: Fix the checkpatch warnings 2016-12-15 9:30 [PATCH 0/7] PM / devfreq: Update the devfreq and devfreq-event device Chanwoo Choi ` (3 preceding siblings ...) 2016-12-15 9:30 ` [PATCH 4/7] PM / devfreq: exynos-ppmu: Show the registred device for ppmu device Chanwoo Choi @ 2016-12-15 9:30 ` Chanwoo Choi 2016-12-15 9:30 ` [PATCH 6/7] PM / devfreq: Modify the device name as devfreq[X] for sysfs Chanwoo Choi 2016-12-15 9:30 ` [PATCH 7/7] PM / devfreq: Simplify the sysfs name of devfreq-event device Chanwoo Choi 6 siblings, 0 replies; 17+ messages in thread From: Chanwoo Choi @ 2016-12-15 9:30 UTC (permalink / raw) To: myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Chanwoo Choi This patch just fixes the checkpatch warnings. Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/devfreq/devfreq.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 47206a21bb90..8e5938c9c7d6 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -538,15 +538,14 @@ struct devfreq *devfreq_add_device(struct device *dev, devfreq = find_device_devfreq(dev); mutex_unlock(&devfreq_list_lock); if (!IS_ERR(devfreq)) { - dev_err(dev, "%s: Unable to create devfreq for the device. It already has one.\n", __func__); + dev_err(dev, "%s: Unable to create devfreq for the device.\n", + __func__); err = -EINVAL; goto err_out; } devfreq = kzalloc(sizeof(struct devfreq), GFP_KERNEL); if (!devfreq) { - dev_err(dev, "%s: Unable to create devfreq for the device\n", - __func__); err = -ENOMEM; goto err_out; } @@ -576,11 +575,13 @@ struct devfreq *devfreq_add_device(struct device *dev, goto err_out; } - devfreq->trans_table = devm_kzalloc(&devfreq->dev, sizeof(unsigned int) * + devfreq->trans_table = devm_kzalloc(&devfreq->dev, + sizeof(unsigned int) * devfreq->profile->max_state * devfreq->profile->max_state, GFP_KERNEL); - devfreq->time_in_state = devm_kzalloc(&devfreq->dev, sizeof(unsigned long) * + devfreq->time_in_state = devm_kzalloc(&devfreq->dev, + sizeof(unsigned long) * devfreq->profile->max_state, GFP_KERNEL); devfreq->last_stat_updated = jiffies; @@ -995,7 +996,7 @@ static ssize_t cur_freq_show(struct device *dev, struct device_attribute *attr, if (devfreq->profile->get_cur_freq && !devfreq->profile->get_cur_freq(devfreq->dev.parent, &freq)) - return sprintf(buf, "%lu\n", freq); + return sprintf(buf, "%lu\n", freq); return sprintf(buf, "%lu\n", devfreq->previous_freq); } -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 6/7] PM / devfreq: Modify the device name as devfreq[X] for sysfs 2016-12-15 9:30 [PATCH 0/7] PM / devfreq: Update the devfreq and devfreq-event device Chanwoo Choi ` (4 preceding siblings ...) 2016-12-15 9:30 ` [PATCH 5/7] PM / devfreq: Fix the checkpatch warnings Chanwoo Choi @ 2016-12-15 9:30 ` Chanwoo Choi 2016-12-15 9:30 ` [PATCH 7/7] PM / devfreq: Simplify the sysfs name of devfreq-event device Chanwoo Choi 6 siblings, 0 replies; 17+ messages in thread From: Chanwoo Choi @ 2016-12-15 9:30 UTC (permalink / raw) To: myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Chanwoo Choi This patch modifies the device name as devfreq[X] for sysfs by using the 'devfreq' prefix word instead of separate device name. On user-space aspect, user would find the some devfreq drvier with 'devfreq[X]' pattern. So, this patch modify the device name as following: - /sys/class/devfreq/[non-standard device name] -> /sys/class/devfreq/devfreq[X] Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/devfreq/devfreq.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c index 8e5938c9c7d6..4bd7a8f71b07 100644 --- a/drivers/devfreq/devfreq.c +++ b/drivers/devfreq/devfreq.c @@ -527,6 +527,7 @@ struct devfreq *devfreq_add_device(struct device *dev, { struct devfreq *devfreq; struct devfreq_governor *governor; + static atomic_t devfreq_no = ATOMIC_INIT(-1); int err = 0; if (!dev || !profile || !governor_name) { @@ -568,7 +569,8 @@ struct devfreq *devfreq_add_device(struct device *dev, mutex_lock(&devfreq->lock); } - dev_set_name(&devfreq->dev, "%s", dev_name(dev)); + dev_set_name(&devfreq->dev, "devfreq%lu", + (unsigned long)atomic_inc_return(&devfreq_no)); err = device_register(&devfreq->dev); if (err) { mutex_unlock(&devfreq->lock); -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 7/7] PM / devfreq: Simplify the sysfs name of devfreq-event device 2016-12-15 9:30 [PATCH 0/7] PM / devfreq: Update the devfreq and devfreq-event device Chanwoo Choi ` (5 preceding siblings ...) 2016-12-15 9:30 ` [PATCH 6/7] PM / devfreq: Modify the device name as devfreq[X] for sysfs Chanwoo Choi @ 2016-12-15 9:30 ` Chanwoo Choi 6 siblings, 0 replies; 17+ messages in thread From: Chanwoo Choi @ 2016-12-15 9:30 UTC (permalink / raw) To: myungjoo.ham, kyungmin.park Cc: rjw, chanwoo, linux-pm, linux-kernel, Chanwoo Choi This patch just removes '.' character from the sysfs name of devfreq-event device as following. Usually, the subsystem uses the similiar naming style such as {framework name}{Number}. - old : /sys/class/devfreq-event/event.X - new : /sys/class/devfreq-event/eventX Signed-off-by: Chanwoo Choi <cw00.choi@samsung.com> --- drivers/devfreq/devfreq-event.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/devfreq/devfreq-event.c b/drivers/devfreq/devfreq-event.c index 9aea2c7ecbe6..39f8704ad664 100644 --- a/drivers/devfreq/devfreq-event.c +++ b/drivers/devfreq/devfreq-event.c @@ -329,7 +329,7 @@ struct devfreq_event_dev *devfreq_event_add_edev(struct device *dev, edev->dev.class = devfreq_event_class; edev->dev.release = devfreq_event_release_edev; - dev_set_name(&edev->dev, "event.%d", atomic_inc_return(&event_no) - 1); + dev_set_name(&edev->dev, "event%d", atomic_inc_return(&event_no) - 1); ret = device_register(&edev->dev); if (ret < 0) { put_device(&edev->dev); -- 1.9.1 ^ permalink raw reply related [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-01-05 16:08 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-12-15 9:30 [PATCH 0/7] PM / devfreq: Update the devfreq and devfreq-event device Chanwoo Choi 2016-12-15 9:30 ` [PATCH 1/7] PM / devfreq: exynos-bus: Add the detailed correlation for Exynos5433 Chanwoo Choi 2016-12-15 9:30 ` [PATCH 2/7] PM / devfreq: exynos-ppmu: Use the regmap interface to handle the registers Chanwoo Choi 2016-12-19 19:47 ` Tobias Jakobi 2016-12-20 2:35 ` Chanwoo Choi 2016-12-20 8:08 ` Tobias Jakobi 2016-12-20 8:26 ` Chanwoo Choi 2016-12-20 8:32 ` Chanwoo Choi 2016-12-20 13:47 ` Tobias Jakobi 2016-12-20 13:47 ` Tobias Jakobi 2016-12-20 19:20 ` Chanwoo Choi 2017-01-05 16:07 ` Tobias Jakobi 2016-12-15 9:30 ` [PATCH 3/7] PM / devfreq: exynos-bus: Print the real clock rate of bus Chanwoo Choi 2016-12-15 9:30 ` [PATCH 4/7] PM / devfreq: exynos-ppmu: Show the registred device for ppmu device Chanwoo Choi 2016-12-15 9:30 ` [PATCH 5/7] PM / devfreq: Fix the checkpatch warnings Chanwoo Choi 2016-12-15 9:30 ` [PATCH 6/7] PM / devfreq: Modify the device name as devfreq[X] for sysfs Chanwoo Choi 2016-12-15 9:30 ` [PATCH 7/7] PM / devfreq: Simplify the sysfs name of devfreq-event device Chanwoo Choi
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.