* [PATCHv4 0/2] Improved perf support for imx53/ppd @ 2018-02-12 12:39 Sebastian Reichel 2018-02-12 12:39 ` [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU Sebastian Reichel 2018-02-12 12:39 ` [PATCHv4 2/2] ARM: dts: imx53: PPD: Enable secure-reg-access Sebastian Reichel 0 siblings, 2 replies; 14+ messages in thread From: Sebastian Reichel @ 2018-02-12 12:39 UTC (permalink / raw) To: Shawn Guo, Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland Cc: Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel Hi, This improves perf on imx53 by adding support for enabling the Secure Debug Enable Register (SDER) SUNIDEN bit. This unlocks new functionality: ppd before patchset# perf stat -e cycles,instructions sleep 1 2>&1 | grep instructions 0 instructions # 0.00 insn per cycle ppd after patchset# perf stat -e cycles,instructions sleep 1 2>&1 | grep instructions 177864 instructions # 0.05 insn per cycle Changes since PATCHV3: * Use readl_relaxed instead of __raw_readl (and the same for writel) * Drop final return in void function * Add Reviewed-by from Fabio to 2nd patch Changes since PATCHv2: * Always enable GPC_DBG_EN when kernel configuration allows using perf to allow cleanup of the arm-pmu driver. Changes since PATCHv1: * Update DTS patch to reference imx53.dtsi's pmu node * Remove a superfluous newline in first patch -- Sebastian Sebastian Reichel (2): ARM: imx53: add secure-reg-access support for PMU ARM: dts: imx53: PPD: Enable secure-reg-access arch/arm/boot/dts/imx53-ppd.dts | 4 ++++ arch/arm/boot/dts/imx53.dtsi | 2 +- arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 43 insertions(+), 2 deletions(-) -- 2.15.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-02-12 12:39 [PATCHv4 0/2] Improved perf support for imx53/ppd Sebastian Reichel @ 2018-02-12 12:39 ` Sebastian Reichel 2018-02-12 12:48 ` Fabio Estevam 2018-02-24 7:45 ` Shawn Guo 2018-02-12 12:39 ` [PATCHv4 2/2] ARM: dts: imx53: PPD: Enable secure-reg-access Sebastian Reichel 1 sibling, 2 replies; 14+ messages in thread From: Sebastian Reichel @ 2018-02-12 12:39 UTC (permalink / raw) To: Shawn Guo, Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland Cc: Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel, Sebastian Reichel On i.MX53 it is necessary to set the DBG_EN bit in the platform GPC register to enable access to PMU counters other than the cycle counter. Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> --- arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c index 07c2e8dca494..658e28604dca 100644 --- a/arch/arm/mach-imx/mach-imx53.c +++ b/arch/arm/mach-imx/mach-imx53.c @@ -28,10 +28,47 @@ static void __init imx53_init_early(void) mxc_set_cpu_type(MXC_CPU_MX53); } +#define MXC_CORTEXA8_PLAT_GPC 0x63fa0004 +#define GPC_DBG_EN BIT(16) + +/* + * This enables the DBGEN bit in ARM_GPC register, which is + * required for accessing some performance counter features. + * Technically it is only required while perf is used, but to + * keep the source code simple we just enable it all the time + * when the kernel configuration allows using the feature. + */ +static void imx53_pmu_init(void) +{ + void __iomem *gpc_reg; + struct device_node *node; + u32 gpc; + + if (!IS_ENABLED(CONFIG_ARM_PMU)) + return; + + node = of_find_compatible_node(NULL, NULL, "arm,cortex-a8-pmu"); + if (!node) + return; + + if (!of_property_read_bool(node, "secure-reg-access")) + return; + + gpc_reg = ioremap(MXC_CORTEXA8_PLAT_GPC, 4); + if (!gpc_reg) { + pr_warning("unable to map GPC to enable perf\n"); + return; + } + + gpc = readl_relaxed(gpc_reg); + gpc |= GPC_DBG_EN; + writel_relaxed(gpc, gpc_reg); +} + static void __init imx53_dt_init(void) { imx_src_init(); - + imx53_pmu_init(); imx_aips_allow_unprivileged_access("fsl,imx53-aipstz"); } -- 2.15.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-02-12 12:39 ` [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU Sebastian Reichel @ 2018-02-12 12:48 ` Fabio Estevam 2018-02-24 7:45 ` Shawn Guo 1 sibling, 0 replies; 14+ messages in thread From: Fabio Estevam @ 2018-02-12 12:48 UTC (permalink / raw) To: Sebastian Reichel Cc: Shawn Guo, Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, linux-kernel, Russell King, Ian Ray, Nandor Han, kernel, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE On Mon, Feb 12, 2018 at 10:39 AM, Sebastian Reichel <sebastian.reichel@collabora.co.uk> wrote: > On i.MX53 it is necessary to set the DBG_EN bit in the > platform GPC register to enable access to PMU counters > other than the cycle counter. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-02-12 12:39 ` [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU Sebastian Reichel 2018-02-12 12:48 ` Fabio Estevam @ 2018-02-24 7:45 ` Shawn Guo 2018-02-26 13:47 ` Sebastian Reichel 1 sibling, 1 reply; 14+ messages in thread From: Shawn Guo @ 2018-02-24 7:45 UTC (permalink / raw) To: Sebastian Reichel Cc: Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel On Mon, Feb 12, 2018 at 01:39:44PM +0100, Sebastian Reichel wrote: > On i.MX53 it is necessary to set the DBG_EN bit in the > platform GPC register to enable access to PMU counters > other than the cycle counter. > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > --- > arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c > index 07c2e8dca494..658e28604dca 100644 > --- a/arch/arm/mach-imx/mach-imx53.c > +++ b/arch/arm/mach-imx/mach-imx53.c > @@ -28,10 +28,47 @@ static void __init imx53_init_early(void) > mxc_set_cpu_type(MXC_CPU_MX53); > } > > +#define MXC_CORTEXA8_PLAT_GPC 0x63fa0004 The base address should be retrieved from device tree. Shawn > +#define GPC_DBG_EN BIT(16) > + > +/* > + * This enables the DBGEN bit in ARM_GPC register, which is > + * required for accessing some performance counter features. > + * Technically it is only required while perf is used, but to > + * keep the source code simple we just enable it all the time > + * when the kernel configuration allows using the feature. > + */ > +static void imx53_pmu_init(void) > +{ > + void __iomem *gpc_reg; > + struct device_node *node; > + u32 gpc; > + > + if (!IS_ENABLED(CONFIG_ARM_PMU)) > + return; > + > + node = of_find_compatible_node(NULL, NULL, "arm,cortex-a8-pmu"); > + if (!node) > + return; > + > + if (!of_property_read_bool(node, "secure-reg-access")) > + return; > + > + gpc_reg = ioremap(MXC_CORTEXA8_PLAT_GPC, 4); > + if (!gpc_reg) { > + pr_warning("unable to map GPC to enable perf\n"); > + return; > + } > + > + gpc = readl_relaxed(gpc_reg); > + gpc |= GPC_DBG_EN; > + writel_relaxed(gpc, gpc_reg); > +} > + > static void __init imx53_dt_init(void) > { > imx_src_init(); > - > + imx53_pmu_init(); > imx_aips_allow_unprivileged_access("fsl,imx53-aipstz"); > } > > -- > 2.15.1 > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-02-24 7:45 ` Shawn Guo @ 2018-02-26 13:47 ` Sebastian Reichel 2018-02-27 1:10 ` Shawn Guo 2018-06-18 4:00 ` Fabio Estevam 0 siblings, 2 replies; 14+ messages in thread From: Sebastian Reichel @ 2018-02-26 13:47 UTC (permalink / raw) To: Shawn Guo Cc: Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 2520 bytes --] Hi Shawn, On Sat, Feb 24, 2018 at 03:45:44PM +0800, Shawn Guo wrote: > On Mon, Feb 12, 2018 at 01:39:44PM +0100, Sebastian Reichel wrote: > > On i.MX53 it is necessary to set the DBG_EN bit in the > > platform GPC register to enable access to PMU counters > > other than the cycle counter. > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > --- > > arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c > > index 07c2e8dca494..658e28604dca 100644 > > --- a/arch/arm/mach-imx/mach-imx53.c > > +++ b/arch/arm/mach-imx/mach-imx53.c > > @@ -28,10 +28,47 @@ static void __init imx53_init_early(void) > > mxc_set_cpu_type(MXC_CPU_MX53); > > } > > > > +#define MXC_CORTEXA8_PLAT_GPC 0x63fa0004 > > The base address should be retrieved from device tree. DT has no entry for 0x63fa0000-0x63fa3fff. iMX53 TRM lists it as "ARM Platform" with 8 platform specific 32 bit registers. Do you think it's worth the trouble adding a new binding? Do you have a suggestion for a compatible value? -- Sebastian > > Shawn > > > +#define GPC_DBG_EN BIT(16) > > + > > +/* > > + * This enables the DBGEN bit in ARM_GPC register, which is > > + * required for accessing some performance counter features. > > + * Technically it is only required while perf is used, but to > > + * keep the source code simple we just enable it all the time > > + * when the kernel configuration allows using the feature. > > + */ > > +static void imx53_pmu_init(void) > > +{ > > + void __iomem *gpc_reg; > > + struct device_node *node; > > + u32 gpc; > > + > > + if (!IS_ENABLED(CONFIG_ARM_PMU)) > > + return; > > + > > + node = of_find_compatible_node(NULL, NULL, "arm,cortex-a8-pmu"); > > + if (!node) > > + return; > > + > > + if (!of_property_read_bool(node, "secure-reg-access")) > > + return; > > + > > + gpc_reg = ioremap(MXC_CORTEXA8_PLAT_GPC, 4); > > + if (!gpc_reg) { > > + pr_warning("unable to map GPC to enable perf\n"); > > + return; > > + } > > + > > + gpc = readl_relaxed(gpc_reg); > > + gpc |= GPC_DBG_EN; > > + writel_relaxed(gpc, gpc_reg); > > +} > > + > > static void __init imx53_dt_init(void) > > { > > imx_src_init(); > > - > > + imx53_pmu_init(); > > imx_aips_allow_unprivileged_access("fsl,imx53-aipstz"); > > } > > > > -- > > 2.15.1 > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-02-26 13:47 ` Sebastian Reichel @ 2018-02-27 1:10 ` Shawn Guo 2018-02-27 10:17 ` Sebastian Reichel 2018-06-18 4:00 ` Fabio Estevam 1 sibling, 1 reply; 14+ messages in thread From: Shawn Guo @ 2018-02-27 1:10 UTC (permalink / raw) To: Sebastian Reichel Cc: Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel On Mon, Feb 26, 2018 at 02:47:41PM +0100, Sebastian Reichel wrote: > Hi Shawn, > > On Sat, Feb 24, 2018 at 03:45:44PM +0800, Shawn Guo wrote: > > On Mon, Feb 12, 2018 at 01:39:44PM +0100, Sebastian Reichel wrote: > > > On i.MX53 it is necessary to set the DBG_EN bit in the > > > platform GPC register to enable access to PMU counters > > > other than the cycle counter. > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > > --- > > > arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c > > > index 07c2e8dca494..658e28604dca 100644 > > > --- a/arch/arm/mach-imx/mach-imx53.c > > > +++ b/arch/arm/mach-imx/mach-imx53.c > > > @@ -28,10 +28,47 @@ static void __init imx53_init_early(void) > > > mxc_set_cpu_type(MXC_CPU_MX53); > > > } > > > > > > +#define MXC_CORTEXA8_PLAT_GPC 0x63fa0004 > > > > The base address should be retrieved from device tree. > > DT has no entry for 0x63fa0000-0x63fa3fff. iMX53 TRM lists it as "ARM Platform" > with 8 platform specific 32 bit registers. Do you think it's worth the trouble > adding a new binding? Do you have a suggestion for a compatible value? Looking at it more closely, I feel that patching every single platform which needs to set up additional register for secure-reg-access support doesn't really scale. Can we have pmu driver do it with a phandle in DT pointing to the register and bit that need to be configured? Shawn > > > +#define GPC_DBG_EN BIT(16) > > > + > > > +/* > > > + * This enables the DBGEN bit in ARM_GPC register, which is > > > + * required for accessing some performance counter features. > > > + * Technically it is only required while perf is used, but to > > > + * keep the source code simple we just enable it all the time > > > + * when the kernel configuration allows using the feature. > > > + */ > > > +static void imx53_pmu_init(void) > > > +{ > > > + void __iomem *gpc_reg; > > > + struct device_node *node; > > > + u32 gpc; > > > + > > > + if (!IS_ENABLED(CONFIG_ARM_PMU)) > > > + return; > > > + > > > + node = of_find_compatible_node(NULL, NULL, "arm,cortex-a8-pmu"); > > > + if (!node) > > > + return; > > > + > > > + if (!of_property_read_bool(node, "secure-reg-access")) > > > + return; > > > + > > > + gpc_reg = ioremap(MXC_CORTEXA8_PLAT_GPC, 4); > > > + if (!gpc_reg) { > > > + pr_warning("unable to map GPC to enable perf\n"); > > > + return; > > > + } > > > + > > > + gpc = readl_relaxed(gpc_reg); > > > + gpc |= GPC_DBG_EN; > > > + writel_relaxed(gpc, gpc_reg); > > > +} > > > + > > > static void __init imx53_dt_init(void) > > > { > > > imx_src_init(); > > > - > > > + imx53_pmu_init(); > > > imx_aips_allow_unprivileged_access("fsl,imx53-aipstz"); > > > } > > > > > > -- > > > 2.15.1 > > > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-02-27 1:10 ` Shawn Guo @ 2018-02-27 10:17 ` Sebastian Reichel 2018-05-25 15:45 ` Sebastian Reichel 2018-05-28 2:26 ` Shawn Guo 0 siblings, 2 replies; 14+ messages in thread From: Sebastian Reichel @ 2018-02-27 10:17 UTC (permalink / raw) To: Shawn Guo Cc: Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 3558 bytes --] Hi, On Tue, Feb 27, 2018 at 09:10:34AM +0800, Shawn Guo wrote: > On Mon, Feb 26, 2018 at 02:47:41PM +0100, Sebastian Reichel wrote: > > On Sat, Feb 24, 2018 at 03:45:44PM +0800, Shawn Guo wrote: > > > On Mon, Feb 12, 2018 at 01:39:44PM +0100, Sebastian Reichel wrote: > > > > On i.MX53 it is necessary to set the DBG_EN bit in the > > > > platform GPC register to enable access to PMU counters > > > > other than the cycle counter. > > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > > > --- > > > > arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c > > > > index 07c2e8dca494..658e28604dca 100644 > > > > --- a/arch/arm/mach-imx/mach-imx53.c > > > > +++ b/arch/arm/mach-imx/mach-imx53.c > > > > @@ -28,10 +28,47 @@ static void __init imx53_init_early(void) > > > > mxc_set_cpu_type(MXC_CPU_MX53); > > > > } > > > > > > > > +#define MXC_CORTEXA8_PLAT_GPC 0x63fa0004 > > > > > > The base address should be retrieved from device tree. > > > > DT has no entry for 0x63fa0000-0x63fa3fff. iMX53 TRM lists it as "ARM Platform" > > with 8 platform specific 32 bit registers. Do you think it's worth the trouble > > adding a new binding? Do you have a suggestion for a compatible value? > > Looking at it more closely, I feel that patching every single platform > which needs to set up additional register for secure-reg-access support > doesn't really scale. Can we have pmu driver do it with a phandle in > DT pointing to the register and bit that need to be configured? The PMU driver used to have a feature for initialising platform specific bits, but it is currently being removed to make the PMU code more maintainable. My understanding is, that it's very uncommon to require platform specific setup to get secure-reg-access working. I.e. it is not needed for newer iMX platforms. -- Sebastian > > Shawn > > > > > +#define GPC_DBG_EN BIT(16) > > > > + > > > > +/* > > > > + * This enables the DBGEN bit in ARM_GPC register, which is > > > > + * required for accessing some performance counter features. > > > > + * Technically it is only required while perf is used, but to > > > > + * keep the source code simple we just enable it all the time > > > > + * when the kernel configuration allows using the feature. > > > > + */ > > > > +static void imx53_pmu_init(void) > > > > +{ > > > > + void __iomem *gpc_reg; > > > > + struct device_node *node; > > > > + u32 gpc; > > > > + > > > > + if (!IS_ENABLED(CONFIG_ARM_PMU)) > > > > + return; > > > > + > > > > + node = of_find_compatible_node(NULL, NULL, "arm,cortex-a8-pmu"); > > > > + if (!node) > > > > + return; > > > > + > > > > + if (!of_property_read_bool(node, "secure-reg-access")) > > > > + return; > > > > + > > > > + gpc_reg = ioremap(MXC_CORTEXA8_PLAT_GPC, 4); > > > > + if (!gpc_reg) { > > > > + pr_warning("unable to map GPC to enable perf\n"); > > > > + return; > > > > + } > > > > + > > > > + gpc = readl_relaxed(gpc_reg); > > > > + gpc |= GPC_DBG_EN; > > > > + writel_relaxed(gpc, gpc_reg); > > > > +} > > > > + > > > > static void __init imx53_dt_init(void) > > > > { > > > > imx_src_init(); > > > > - > > > > + imx53_pmu_init(); > > > > imx_aips_allow_unprivileged_access("fsl,imx53-aipstz"); > > > > } > > > > > > > > -- > > > > 2.15.1 > > > > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-02-27 10:17 ` Sebastian Reichel @ 2018-05-25 15:45 ` Sebastian Reichel 2018-05-28 2:26 ` Shawn Guo 1 sibling, 0 replies; 14+ messages in thread From: Sebastian Reichel @ 2018-05-25 15:45 UTC (permalink / raw) To: Shawn Guo Cc: Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 2420 bytes --] Hi Shawn, On Tue, Feb 27, 2018 at 11:17:12AM +0100, Sebastian Reichel wrote: > Hi, > > On Tue, Feb 27, 2018 at 09:10:34AM +0800, Shawn Guo wrote: > > On Mon, Feb 26, 2018 at 02:47:41PM +0100, Sebastian Reichel wrote: > > > On Sat, Feb 24, 2018 at 03:45:44PM +0800, Shawn Guo wrote: > > > > On Mon, Feb 12, 2018 at 01:39:44PM +0100, Sebastian Reichel wrote: > > > > > On i.MX53 it is necessary to set the DBG_EN bit in the > > > > > platform GPC register to enable access to PMU counters > > > > > other than the cycle counter. > > > > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > > > > --- > > > > > arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c > > > > > index 07c2e8dca494..658e28604dca 100644 > > > > > --- a/arch/arm/mach-imx/mach-imx53.c > > > > > +++ b/arch/arm/mach-imx/mach-imx53.c > > > > > @@ -28,10 +28,47 @@ static void __init imx53_init_early(void) > > > > > mxc_set_cpu_type(MXC_CPU_MX53); > > > > > } > > > > > > > > > > +#define MXC_CORTEXA8_PLAT_GPC 0x63fa0004 > > > > > > > > The base address should be retrieved from device tree. > > > > > > DT has no entry for 0x63fa0000-0x63fa3fff. iMX53 TRM lists it as "ARM Platform" > > > with 8 platform specific 32 bit registers. Do you think it's worth the trouble > > > adding a new binding? Do you have a suggestion for a compatible value? > > > > Looking at it more closely, I feel that patching every single platform > > which needs to set up additional register for secure-reg-access support > > doesn't really scale. Can we have pmu driver do it with a phandle in > > DT pointing to the register and bit that need to be configured? > > The PMU driver used to have a feature for initialising platform > specific bits, but it is currently being removed to make the PMU > code more maintainable. My understanding is, that it's very uncommon > to require platform specific setup to get secure-reg-access working. > E.g. it is not needed for newer iMX platforms. You never replied to this one. Are you fine with adding the function? This is kind of a deadlock situation with neither you wanting to enable the platform bit, nor the generic PMU driver. -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-02-27 10:17 ` Sebastian Reichel 2018-05-25 15:45 ` Sebastian Reichel @ 2018-05-28 2:26 ` Shawn Guo 2018-05-28 6:41 ` Sebastian Reichel 1 sibling, 1 reply; 14+ messages in thread From: Shawn Guo @ 2018-05-28 2:26 UTC (permalink / raw) To: Sebastian Reichel Cc: Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel On Tue, Feb 27, 2018 at 11:17:12AM +0100, Sebastian Reichel wrote: > Hi, > > On Tue, Feb 27, 2018 at 09:10:34AM +0800, Shawn Guo wrote: > > On Mon, Feb 26, 2018 at 02:47:41PM +0100, Sebastian Reichel wrote: > > > On Sat, Feb 24, 2018 at 03:45:44PM +0800, Shawn Guo wrote: > > > > On Mon, Feb 12, 2018 at 01:39:44PM +0100, Sebastian Reichel wrote: > > > > > On i.MX53 it is necessary to set the DBG_EN bit in the > > > > > platform GPC register to enable access to PMU counters > > > > > other than the cycle counter. > > > > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > > > > --- > > > > > arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c > > > > > index 07c2e8dca494..658e28604dca 100644 > > > > > --- a/arch/arm/mach-imx/mach-imx53.c > > > > > +++ b/arch/arm/mach-imx/mach-imx53.c > > > > > @@ -28,10 +28,47 @@ static void __init imx53_init_early(void) > > > > > mxc_set_cpu_type(MXC_CPU_MX53); > > > > > } > > > > > > > > > > +#define MXC_CORTEXA8_PLAT_GPC 0x63fa0004 > > > > > > > > The base address should be retrieved from device tree. > > > > > > DT has no entry for 0x63fa0000-0x63fa3fff. iMX53 TRM lists it as "ARM Platform" > > > with 8 platform specific 32 bit registers. Do you think it's worth the trouble > > > adding a new binding? Do you have a suggestion for a compatible value? > > > > Looking at it more closely, I feel that patching every single platform > > which needs to set up additional register for secure-reg-access support > > doesn't really scale. Can we have pmu driver do it with a phandle in > > DT pointing to the register and bit that need to be configured? > > The PMU driver used to have a feature for initialising platform > specific bits, but it is currently being removed to make the PMU > code more maintainable. My understanding is, that it's very uncommon > to require platform specific setup to get secure-reg-access working. > I.e. it is not needed for newer iMX platforms. Are you saying this is a very specific setup required by i.MX53 only? In that case, I can live with it. Shawn ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-05-28 2:26 ` Shawn Guo @ 2018-05-28 6:41 ` Sebastian Reichel 2018-05-28 7:20 ` Shawn Guo 0 siblings, 1 reply; 14+ messages in thread From: Sebastian Reichel @ 2018-05-28 6:41 UTC (permalink / raw) To: Shawn Guo Cc: Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 2728 bytes --] Hi, On Mon, May 28, 2018 at 10:26:54AM +0800, Shawn Guo wrote: > On Tue, Feb 27, 2018 at 11:17:12AM +0100, Sebastian Reichel wrote: > > Hi, > > > > On Tue, Feb 27, 2018 at 09:10:34AM +0800, Shawn Guo wrote: > > > On Mon, Feb 26, 2018 at 02:47:41PM +0100, Sebastian Reichel wrote: > > > > On Sat, Feb 24, 2018 at 03:45:44PM +0800, Shawn Guo wrote: > > > > > On Mon, Feb 12, 2018 at 01:39:44PM +0100, Sebastian Reichel wrote: > > > > > > On i.MX53 it is necessary to set the DBG_EN bit in the > > > > > > platform GPC register to enable access to PMU counters > > > > > > other than the cycle counter. > > > > > > > > > > > > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> > > > > > > --- > > > > > > arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > > > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c > > > > > > index 07c2e8dca494..658e28604dca 100644 > > > > > > --- a/arch/arm/mach-imx/mach-imx53.c > > > > > > +++ b/arch/arm/mach-imx/mach-imx53.c > > > > > > @@ -28,10 +28,47 @@ static void __init imx53_init_early(void) > > > > > > mxc_set_cpu_type(MXC_CPU_MX53); > > > > > > } > > > > > > > > > > > > +#define MXC_CORTEXA8_PLAT_GPC 0x63fa0004 > > > > > > > > > > The base address should be retrieved from device tree. > > > > > > > > DT has no entry for 0x63fa0000-0x63fa3fff. iMX53 TRM lists it as "ARM Platform" > > > > with 8 platform specific 32 bit registers. Do you think it's worth the trouble > > > > adding a new binding? Do you have a suggestion for a compatible value? > > > > > > Looking at it more closely, I feel that patching every single platform > > > which needs to set up additional register for secure-reg-access support > > > doesn't really scale. Can we have pmu driver do it with a phandle in > > > DT pointing to the register and bit that need to be configured? > > > > The PMU driver used to have a feature for initialising platform > > specific bits, but it is currently being removed to make the PMU > > code more maintainable. My understanding is, that it's very uncommon > > to require platform specific setup to get secure-reg-access working. > > I.e. it is not needed for newer iMX platforms. > > Are you saying this is a very specific setup required by i.MX53 only? Yes, all other SoCs supported by Linux ARM PMU counters driver can just use the registers without having to enable platform specific bits first. > In that case, I can live with it. What about the DT node? I did not add it, since this is a i.MX53 specific workaround anyways. -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-05-28 6:41 ` Sebastian Reichel @ 2018-05-28 7:20 ` Shawn Guo 2018-05-28 15:50 ` Sebastian Reichel 0 siblings, 1 reply; 14+ messages in thread From: Shawn Guo @ 2018-05-28 7:20 UTC (permalink / raw) To: Sebastian Reichel Cc: Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel On Mon, May 28, 2018 at 08:41:31AM +0200, Sebastian Reichel wrote: > > Are you saying this is a very specific setup required by i.MX53 only? > > Yes, all other SoCs supported by Linux ARM PMU counters driver can > just use the registers without having to enable platform specific > bits first. > > > In that case, I can live with it. > > What about the DT node? I did not add it, since this is a i.MX53 > specific workaround anyways. What you are adding here is secure-reg-access property, which has an defined meaning in PMU binding doc. I'm not really sure if it's appropriate to use the property as a condition for DBGEN bit setup. Or can we set up the bit regardless of the property? Shawn ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-05-28 7:20 ` Shawn Guo @ 2018-05-28 15:50 ` Sebastian Reichel 0 siblings, 0 replies; 14+ messages in thread From: Sebastian Reichel @ 2018-05-28 15:50 UTC (permalink / raw) To: Shawn Guo Cc: Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland, Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel [-- Attachment #1: Type: text/plain, Size: 1404 bytes --] Hi, On Mon, May 28, 2018 at 03:20:35PM +0800, Shawn Guo wrote: > On Mon, May 28, 2018 at 08:41:31AM +0200, Sebastian Reichel wrote: > > > Are you saying this is a very specific setup required by i.MX53 only? > > > > Yes, all other SoCs supported by Linux ARM PMU counters driver can > > just use the registers without having to enable platform specific > > bits first. > > > > > In that case, I can live with it. > > > > What about the DT node? I did not add it, since this is a i.MX53 > > specific workaround anyways. > > What you are adding here is secure-reg-access property, which has an > defined meaning in PMU binding doc. I'm not really sure if it's > appropriate to use the property as a condition for DBGEN bit setup. > Or can we set up the bit regardless of the property? The description for DBGEN bit is: > Debug enable. This allows the user to manually activate clocks > within the debug system. This register bit directly controls the > platform's dbgen_out output signal which connects to the DAP_SYS to > enable all debug clocks. Once enabled, the clocks cannot be disabled > except by asserting the disable_trace input of the DAP_SYS. I only enable this bit when the kernel configuration allows using the PMU, since otherwise we do not need the clocks in the debug system. This limits any potential side-effects of this patchset. -- Sebastian [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU 2018-02-26 13:47 ` Sebastian Reichel 2018-02-27 1:10 ` Shawn Guo @ 2018-06-18 4:00 ` Fabio Estevam 1 sibling, 0 replies; 14+ messages in thread From: Fabio Estevam @ 2018-06-18 4:00 UTC (permalink / raw) To: Sebastian Reichel, Martin Fuzzey Cc: Shawn Guo, Mark Rutland, Will Deacon, Russell King, linux-kernel, Ian Ray, Sascha Hauer, Nandor Han, Fabio Estevam, kernel, moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE Hi Sebastian, [Adding Martin on Cc who also tried to enable DBGEN bit in a previous patch] On Mon, Feb 26, 2018 at 10:47 AM, Sebastian Reichel <sebastian.reichel@collabora.co.uk> wrote: > Hi Shawn, > > On Sat, Feb 24, 2018 at 03:45:44PM +0800, Shawn Guo wrote: >> On Mon, Feb 12, 2018 at 01:39:44PM +0100, Sebastian Reichel wrote: >> > On i.MX53 it is necessary to set the DBG_EN bit in the >> > platform GPC register to enable access to PMU counters >> > other than the cycle counter. >> > >> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> >> > --- >> > arch/arm/mach-imx/mach-imx53.c | 39 ++++++++++++++++++++++++++++++++++++++- >> > 1 file changed, 38 insertions(+), 1 deletion(-) >> > >> > diff --git a/arch/arm/mach-imx/mach-imx53.c b/arch/arm/mach-imx/mach-imx53.c >> > index 07c2e8dca494..658e28604dca 100644 >> > --- a/arch/arm/mach-imx/mach-imx53.c >> > +++ b/arch/arm/mach-imx/mach-imx53.c >> > @@ -28,10 +28,47 @@ static void __init imx53_init_early(void) >> > mxc_set_cpu_type(MXC_CPU_MX53); >> > } >> > >> > +#define MXC_CORTEXA8_PLAT_GPC 0x63fa0004 >> >> The base address should be retrieved from device tree. > > DT has no entry for 0x63fa0000-0x63fa3fff. iMX53 TRM lists it as "ARM Platform" > with 8 platform specific 32 bit registers. Do you think it's worth the trouble > adding a new binding? Do you have a suggestion for a compatible value? I also think we should add a compatible string for the "ARM platform" region. This way both mx51 and mx53 could retrieve the base address from the device tree. What about: --- a/arch/arm/boot/dts/imx53.dtsi +++ b/arch/arm/boot/dts/imx53.dtsi @@ -672,6 +672,11 @@ status = "disabled"; }; + arm_plat: arm-plat@63fa0000 { + compatible = "fsl,imx53-arm-plat", "fsl,imx51-arm-plat"; + reg = <0x63fa0000 0x4000>; + }; + owire: owire@63fa4000 { compatible = "fsl,imx53-owire", "fsl,imx21-owire"; reg = <0x63fa4000 0x4000>; ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCHv4 2/2] ARM: dts: imx53: PPD: Enable secure-reg-access 2018-02-12 12:39 [PATCHv4 0/2] Improved perf support for imx53/ppd Sebastian Reichel 2018-02-12 12:39 ` [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU Sebastian Reichel @ 2018-02-12 12:39 ` Sebastian Reichel 1 sibling, 0 replies; 14+ messages in thread From: Sebastian Reichel @ 2018-02-12 12:39 UTC (permalink / raw) To: Shawn Guo, Sascha Hauer, Fabio Estevam, Will Deacon, Mark Rutland Cc: Russell King, Ian Ray, Nandor Han, linux-arm-kernel, linux-kernel, kernel, Sebastian Reichel Add secure-reg-access on PPD device tree to enable PMU and hardware counters for perf. Reviewed-by: Fabio Estevam <fabio.estevam@nxp.com> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> --- arch/arm/boot/dts/imx53-ppd.dts | 4 ++++ arch/arm/boot/dts/imx53.dtsi | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/arch/arm/boot/dts/imx53-ppd.dts b/arch/arm/boot/dts/imx53-ppd.dts index dd1b524e5074..3c87e0d73f63 100644 --- a/arch/arm/boot/dts/imx53-ppd.dts +++ b/arch/arm/boot/dts/imx53-ppd.dts @@ -562,6 +562,10 @@ }; }; +&pmu { + secure-reg-access; +}; + &pwm1 { pinctrl-names = "default"; pinctrl-0 = <&pinctrl_pwm1>; diff --git a/arch/arm/boot/dts/imx53.dtsi b/arch/arm/boot/dts/imx53.dtsi index 1040251f2951..7df4853dc771 100644 --- a/arch/arm/boot/dts/imx53.dtsi +++ b/arch/arm/boot/dts/imx53.dtsi @@ -116,7 +116,7 @@ }; }; - pmu { + pmu: pmu { compatible = "arm,cortex-a8-pmu"; interrupt-parent = <&tzic>; interrupts = <77>; -- 2.15.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
end of thread, other threads:[~2018-06-18 4:00 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-02-12 12:39 [PATCHv4 0/2] Improved perf support for imx53/ppd Sebastian Reichel 2018-02-12 12:39 ` [PATCHv4 1/2] ARM: imx53: add secure-reg-access support for PMU Sebastian Reichel 2018-02-12 12:48 ` Fabio Estevam 2018-02-24 7:45 ` Shawn Guo 2018-02-26 13:47 ` Sebastian Reichel 2018-02-27 1:10 ` Shawn Guo 2018-02-27 10:17 ` Sebastian Reichel 2018-05-25 15:45 ` Sebastian Reichel 2018-05-28 2:26 ` Shawn Guo 2018-05-28 6:41 ` Sebastian Reichel 2018-05-28 7:20 ` Shawn Guo 2018-05-28 15:50 ` Sebastian Reichel 2018-06-18 4:00 ` Fabio Estevam 2018-02-12 12:39 ` [PATCHv4 2/2] ARM: dts: imx53: PPD: Enable secure-reg-access Sebastian Reichel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).