| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CANLsYkwWWcrQPXWHZM=B7GBDQ4SyiB9cchaXPQFzvOKQUVSMTA@mail.gmail.com>
Date: Tue, 3 Jun 2014 10:37:06 -0600
From: Mathieu Poirier <mathieu.poirier@...aro.org>
To: Daniel Thompson <daniel.thompson@...aro.org>
Cc: Linus Walleij <linus.walleij@...aro.org>,
Will Deacon <will.deacon@....com>,
Arve Hjønnevåg <arve@...roid.com>,
John Stultz <john.stultz@...aro.org>,
Pratik Patel <pratikp@...eaurora.org>,
Vikas Varshney <varshney@...com>, Al Grant <Al.Grant@....com>,
Jonas Svennebring <jonas.svennebring@...gotech.com>,
James King <james.king@...aro.org>,
Panchaxari Prasannamurthy Tumkur
<panchaxari.prasannamurthy@...aro.org>,
Arnd Bergmann <arnd@...aro.org>,
Marcin Jabrzyk <marcin.jabrzyk@...il.com>,
r.sengupta@...sung.com, Robert Marklund <robbelibobban@...il.com>,
Patch Tracking <patches@...aro.org>,
Russell King - ARM Linux <linux@....linux.org.uk>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH 07/11] coresight: add CoreSight ETM driver
Thanks for the review. Please see comments in-lined.
Mathieu
On 3 June 2014 04:26, Daniel Thompson <daniel.thompson@...aro.org> wrote:
> On 30/05/14 14:43, mathieu.poirier@...aro.org wrote:
>> diff --git a/drivers/coresight/coresight-etm-cp14.c b/drivers/coresight/coresight-etm-cp14.c
>> new file mode 100644
>> index 0000000..0088bbb
>> --- /dev/null
>> +++ b/drivers/coresight/coresight-etm-cp14.c
>> @@ -0,0 +1,511 @@
>> +/* Copyright (c) 2012, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/types.h>
>> +#include <linux/bug.h>
>> +#include <asm/hardware/cp14.h>
>> +
>> +static unsigned int etm_read_reg(uint32_t reg)
>> +{
>> + switch (reg) {
>> + case 0x0:
>
> Shouldn't this be:
>
> case ETMCR/4:
>
> Or an equivalent macro? Given the memory mappings are already spelt out
> in another file it seems a shame to restate them again.
>
>> + return etm_read(ETMCR);
>
> Maybe we could even condense the mapping with something like:
>
> #define CASE_MAP_MM_READ_TO_CPIO(x) case (x)/4: return etm_read(x)
>
> CASE_MAP_MM_READ_TO_CPIO(ETMCR);
> CASE_MAP_MM_READ_TO_CPIO(ETMCCR);
> CASE_MAP_MM_READ_TO_CPIO(ETMTRIGGER);
> ...
>
> Note that the macro may not be perfect since it untested and I can't
> remember how it will interact with the token pasting in etm_read(x).
> Howevver but a macro with this interface can definitely be written.
I agree, we can do better.
>
>> + case 0x1:
>> + return etm_read(ETMCCR);
>> + case 0x2:
>> + return etm_read(ETMTRIGGER);
>> + case 0x4:
>> + return etm_read(ETMSR);
>> + case 0x5:
>> + return etm_read(ETMSCR);
>> + case 0x6:
>> + return etm_read(ETMTSSCR);
>> + case 0x8:
>> + return etm_read(ETMTEEVR);
>> + case 0x9:
>> + return etm_read(ETMTECR1);
>> + case 0xB:
>> + return etm_read(ETMFFLR);
>> + case 0x10:
>> + return etm_read(ETMACVR0);
>> + case 0x11:
>> + return etm_read(ETMACVR1);
>> + case 0x12:
>> + return etm_read(ETMACVR2);
>> + case 0x13:
>> + return etm_read(ETMACVR3);
>> + case 0x14:
>> + return etm_read(ETMACVR4);
>> + case 0x15:
>> + return etm_read(ETMACVR5);
>> + case 0x16:
>> + return etm_read(ETMACVR6);
>> + case 0x17:
>> + return etm_read(ETMACVR7);
>> + case 0x18:
>> + return etm_read(ETMACVR8);
>> + case 0x19:
>> + return etm_read(ETMACVR9);
>> + case 0x1A:
>> + return etm_read(ETMACVR10);
>> + case 0x1B:
>> + return etm_read(ETMACVR11);
>> + case 0x1C:
>> + return etm_read(ETMACVR12);
>> + case 0x1D:
>> + return etm_read(ETMACVR13);
>> + case 0x1E:
>> + return etm_read(ETMACVR14);
>> + case 0x1F:
>> + return etm_read(ETMACVR15);
>> + case 0x20:
>> + return etm_read(ETMACTR0);
>> + case 0x21:
>> + return etm_read(ETMACTR1);
>> + case 0x22:
>> + return etm_read(ETMACTR2);
>> + case 0x23:
>> + return etm_read(ETMACTR3);
>> + case 0x24:
>> + return etm_read(ETMACTR4);
>> + case 0x25:
>> + return etm_read(ETMACTR5);
>> + case 0x26:
>> + return etm_read(ETMACTR6);
>> + case 0x27:
>> + return etm_read(ETMACTR7);
>> + case 0x28:
>> + return etm_read(ETMACTR8);
>> + case 0x29:
>> + return etm_read(ETMACTR9);
>> + case 0x2A:
>> + return etm_read(ETMACTR10);
>> + case 0x2B:
>> + return etm_read(ETMACTR11);
>> + case 0x2C:
>> + return etm_read(ETMACTR12);
>> + case 0x2D:
>> + return etm_read(ETMACTR13);
>> + case 0x2E:
>> + return etm_read(ETMACTR14);
>> + case 0x2F:
>> + return etm_read(ETMACTR15);
>> + case 0x50:
>> + return etm_read(ETMCNTRLDVR0);
>> + case 0x51:
>> + return etm_read(ETMCNTRLDVR1);
>> + case 0x52:
>> + return etm_read(ETMCNTRLDVR2);
>> + case 0x53:
>> + return etm_read(ETMCNTRLDVR3);
>> + case 0x54:
>> + return etm_read(ETMCNTENR0);
>> + case 0x55:
>> + return etm_read(ETMCNTENR1);
>> + case 0x56:
>> + return etm_read(ETMCNTENR2);
>> + case 0x57:
>> + return etm_read(ETMCNTENR3);
>> + case 0x58:
>> + return etm_read(ETMCNTRLDEVR0);
>> + case 0x59:
>> + return etm_read(ETMCNTRLDEVR1);
>> + case 0x5A:
>> + return etm_read(ETMCNTRLDEVR2);
>> + case 0x5B:
>> + return etm_read(ETMCNTRLDEVR3);
>> + case 0x5C:
>> + return etm_read(ETMCNTVR0);
>> + case 0x5D:
>> + return etm_read(ETMCNTVR1);
>> + case 0x5E:
>> + return etm_read(ETMCNTVR2);
>> + case 0x5F:
>> + return etm_read(ETMCNTVR3);
>> + case 0x60:
>> + return etm_read(ETMSQ12EVR);
>> + case 0x61:
>> + return etm_read(ETMSQ21EVR);
>> + case 0x62:
>> + return etm_read(ETMSQ23EVR);
>> + case 0x63:
>> + return etm_read(ETMSQ31EVR);
>> + case 0x64:
>> + return etm_read(ETMSQ32EVR);
>> + case 0x65:
>> + return etm_read(ETMSQ13EVR);
>> + case 0x67:
>> + return etm_read(ETMSQR);
>> + case 0x68:
>> + return etm_read(ETMEXTOUTEVR0);
>> + case 0x69:
>> + return etm_read(ETMEXTOUTEVR1);
>> + case 0x6A:
>> + return etm_read(ETMEXTOUTEVR2);
>> + case 0x6B:
>> + return etm_read(ETMEXTOUTEVR3);
>> + case 0x6C:
>> + return etm_read(ETMCIDCVR0);
>> + case 0x6D:
>> + return etm_read(ETMCIDCVR1);
>> + case 0x6E:
>> + return etm_read(ETMCIDCVR2);
>> + case 0x6F:
>> + return etm_read(ETMCIDCMR);
>> + case 0x70:
>> + return etm_read(ETMIMPSPEC0);
>> + case 0x71:
>> + return etm_read(ETMIMPSPEC1);
>> + case 0x72:
>> + return etm_read(ETMIMPSPEC2);
>> + case 0x73:
>> + return etm_read(ETMIMPSPEC3);
>> + case 0x74:
>> + return etm_read(ETMIMPSPEC4);
>> + case 0x75:
>> + return etm_read(ETMIMPSPEC5);
>> + case 0x76:
>> + return etm_read(ETMIMPSPEC6);
>> + case 0x77:
>> + return etm_read(ETMIMPSPEC7);
>> + case 0x78:
>> + return etm_read(ETMSYNCFR);
>> + case 0x79:
>> + return etm_read(ETMIDR);
>> + case 0x7A:
>> + return etm_read(ETMCCER);
>> + case 0x7B:
>> + return etm_read(ETMEXTINSELR);
>> + case 0x7C:
>> + return etm_read(ETMTESSEICR);
>> + case 0x7D:
>> + return etm_read(ETMEIBCR);
>> + case 0x7E:
>> + return etm_read(ETMTSEVR);
>> + case 0x7F:
>> + return etm_read(ETMAUXCR);
>> + case 0x80:
>> + return etm_read(ETMTRACEIDR);
>> + case 0x90:
>> + return etm_read(ETMVMIDCVR);
>> + case 0xC1:
>> + return etm_read(ETMOSLSR);
>> + case 0xC2:
>> + return etm_read(ETMOSSRR);
>> + case 0xC4:
>> + return etm_read(ETMPDCR);
>> + case 0xC5:
>> + return etm_read(ETMPDSR);
>> + default:
>> + WARN(1, "invalid CP14 access to ETM reg: %lx",
>> + (unsigned long)reg);
>> + return 0;
>> + }
>> +}
>> +
>> +static void etm_write_reg(uint32_t val, uint32_t reg)
>> +{
>> + switch (reg) {
>> + case 0x0:
>> + etm_write(val, ETMCR);
>
> Same comment as etm_read_reg but with a different macro.
>
>> + return;
>> + case 0x2:
>> + etm_write(val, ETMTRIGGER);
>> + return;
>> + case 0x4:
>> + etm_write(val, ETMSR);
>> + return;
>> + case 0x6:
>> + etm_write(val, ETMTSSCR);
>> + return;
>> + case 0x8:
>> + etm_write(val, ETMTEEVR);
>> + return;
>> + case 0x9:
>> + etm_write(val, ETMTECR1);
>> + return;
>> + case 0xB:
>> + etm_write(val, ETMFFLR);
>> + return;
>> + case 0x10:
>> + etm_write(val, ETMACVR0);
>> + return;
>> + case 0x11:
>> + etm_write(val, ETMACVR1);
>> + return;
>> + case 0x12:
>> + etm_write(val, ETMACVR2);
>> + return;
>> + case 0x13:
>> + etm_write(val, ETMACVR3);
>> + return;
>> + case 0x14:
>> + etm_write(val, ETMACVR4);
>> + return;
>> + case 0x15:
>> + etm_write(val, ETMACVR5);
>> + return;
>> + case 0x16:
>> + etm_write(val, ETMACVR6);
>> + return;
>> + case 0x17:
>> + etm_write(val, ETMACVR7);
>> + return;
>> + case 0x18:
>> + etm_write(val, ETMACVR8);
>> + return;
>> + case 0x19:
>> + etm_write(val, ETMACVR9);
>> + return;
>> + case 0x1A:
>> + etm_write(val, ETMACVR10);
>> + return;
>> + case 0x1B:
>> + etm_write(val, ETMACVR11);
>> + return;
>> + case 0x1C:
>> + etm_write(val, ETMACVR12);
>> + return;
>> + case 0x1D:
>> + etm_write(val, ETMACVR13);
>> + return;
>> + case 0x1E:
>> + etm_write(val, ETMACVR14);
>> + return;
>> + case 0x1F:
>> + etm_write(val, ETMACVR15);
>> + return;
>> + case 0x20:
>> + etm_write(val, ETMACTR0);
>> + return;
>> + case 0x21:
>> + etm_write(val, ETMACTR1);
>> + return;
>> + case 0x22:
>> + etm_write(val, ETMACTR2);
>> + return;
>> + case 0x23:
>> + etm_write(val, ETMACTR3);
>> + return;
>> + case 0x24:
>> + etm_write(val, ETMACTR4);
>> + return;
>> + case 0x25:
>> + etm_write(val, ETMACTR5);
>> + return;
>> + case 0x26:
>> + etm_write(val, ETMACTR6);
>> + return;
>> + case 0x27:
>> + etm_write(val, ETMACTR7);
>> + return;
>> + case 0x28:
>> + etm_write(val, ETMACTR8);
>> + return;
>> + case 0x29:
>> + etm_write(val, ETMACTR9);
>> + return;
>> + case 0x2A:
>> + etm_write(val, ETMACTR10);
>> + return;
>> + case 0x2B:
>> + etm_write(val, ETMACTR11);
>> + return;
>> + case 0x2C:
>> + etm_write(val, ETMACTR12);
>> + return;
>> + case 0x2D:
>> + etm_write(val, ETMACTR13);
>> + return;
>> + case 0x2E:
>> + etm_write(val, ETMACTR14);
>> + return;
>> + case 0x2F:
>> + etm_write(val, ETMACTR15);
>> + return;
>> + case 0x50:
>> + etm_write(val, ETMCNTRLDVR0);
>> + return;
>> + case 0x51:
>> + etm_write(val, ETMCNTRLDVR1);
>> + return;
>> + case 0x52:
>> + etm_write(val, ETMCNTRLDVR2);
>> + return;
>> + case 0x53:
>> + etm_write(val, ETMCNTRLDVR3);
>> + return;
>> + case 0x54:
>> + etm_write(val, ETMCNTENR0);
>> + return;
>> + case 0x55:
>> + etm_write(val, ETMCNTENR1);
>> + return;
>> + case 0x56:
>> + etm_write(val, ETMCNTENR2);
>> + return;
>> + case 0x57:
>> + etm_write(val, ETMCNTENR3);
>> + return;
>> + case 0x58:
>> + etm_write(val, ETMCNTRLDEVR0);
>> + return;
>> + case 0x59:
>> + etm_write(val, ETMCNTRLDEVR1);
>> + return;
>> + case 0x5A:
>> + etm_write(val, ETMCNTRLDEVR2);
>> + return;
>> + case 0x5B:
>> + etm_write(val, ETMCNTRLDEVR3);
>> + return;
>> + case 0x5C:
>> + etm_write(val, ETMCNTVR0);
>> + return;
>> + case 0x5D:
>> + etm_write(val, ETMCNTVR1);
>> + return;
>> + case 0x5E:
>> + etm_write(val, ETMCNTVR2);
>> + return;
>> + case 0x5F:
>> + etm_write(val, ETMCNTVR3);
>> + return;
>> + case 0x60:
>> + etm_write(val, ETMSQ12EVR);
>> + return;
>> + case 0x61:
>> + etm_write(val, ETMSQ21EVR);
>> + return;
>> + case 0x62:
>> + etm_write(val, ETMSQ23EVR);
>> + return;
>> + case 0x63:
>> + etm_write(val, ETMSQ31EVR);
>> + return;
>> + case 0x64:
>> + etm_write(val, ETMSQ32EVR);
>> + return;
>> + case 0x65:
>> + etm_write(val, ETMSQ13EVR);
>> + return;
>> + case 0x67:
>> + etm_write(val, ETMSQR);
>> + return;
>> + case 0x68:
>> + etm_write(val, ETMEXTOUTEVR0);
>> + return;
>> + case 0x69:
>> + etm_write(val, ETMEXTOUTEVR1);
>> + return;
>> + case 0x6A:
>> + etm_write(val, ETMEXTOUTEVR2);
>> + return;
>> + case 0x6B:
>> + etm_write(val, ETMEXTOUTEVR3);
>> + return;
>> + case 0x6C:
>> + etm_write(val, ETMCIDCVR0);
>> + return;
>> + case 0x6D:
>> + etm_write(val, ETMCIDCVR1);
>> + return;
>> + case 0x6E:
>> + etm_write(val, ETMCIDCVR2);
>> + return;
>> + case 0x6F:
>> + etm_write(val, ETMCIDCMR);
>> + return;
>> + case 0x70:
>> + etm_write(val, ETMIMPSPEC0);
>> + return;
>> + case 0x71:
>> + etm_write(val, ETMIMPSPEC1);
>> + return;
>> + case 0x72:
>> + etm_write(val, ETMIMPSPEC2);
>> + return;
>> + case 0x73:
>> + etm_write(val, ETMIMPSPEC3);
>> + return;
>> + case 0x74:
>> + etm_write(val, ETMIMPSPEC4);
>> + return;
>> + case 0x75:
>> + etm_write(val, ETMIMPSPEC5);
>> + return;
>> + case 0x76:
>> + etm_write(val, ETMIMPSPEC6);
>> + return;
>> + case 0x77:
>> + etm_write(val, ETMIMPSPEC7);
>> + return;
>> + case 0x78:
>> + etm_write(val, ETMSYNCFR);
>> + return;
>> + case 0x7B:
>> + etm_write(val, ETMEXTINSELR);
>> + return;
>> + case 0x7C:
>> + etm_write(val, ETMTESSEICR);
>> + return;
>> + case 0x7D:
>> + etm_write(val, ETMEIBCR);
>> + return;
>> + case 0x7E:
>> + etm_write(val, ETMTSEVR);
>> + return;
>> + case 0x7F:
>> + etm_write(val, ETMAUXCR);
>> + return;
>> + case 0x80:
>> + etm_write(val, ETMTRACEIDR);
>> + return;
>> + case 0x90:
>> + etm_write(val, ETMVMIDCVR);
>> + return;
>> + case 0xC0:
>> + etm_write(val, ETMOSLAR);
>> + return;
>> + case 0xC2:
>> + etm_write(val, ETMOSSRR);
>> + return;
>> + case 0xC4:
>> + etm_write(val, ETMPDCR);
>> + return;
>> + case 0xC5:
>> + etm_write(val, ETMPDSR);
>> + return;
>> + default:
>> + WARN(1, "invalid CP14 access to ETM reg: %lx",
>> + (unsigned long)reg);
>> + return;
>> + }
>> +}
>> +
>> +static inline uint32_t offset_to_reg_num(uint32_t off)
>> +{
>> + return off >> 2;
>> +}
>> +
>> +unsigned int etm_readl_cp14(uint32_t off)
>> +{
>> + uint32_t reg = offset_to_reg_num(off);
>> + return etm_read_reg(reg);
>> +}
>> +
>> +void etm_writel_cp14(uint32_t val, uint32_t off)
>> +{
>> + uint32_t reg = offset_to_reg_num(off);
>> + etm_write_reg(val, reg);
>> +}
>
> Revisiting previous comments... maybe we don't have to divide the MM
> constants by four either? We could just not divide them by four here.
>
>
>> diff --git a/drivers/coresight/coresight-etm.c b/drivers/coresight/coresight-etm.c
>> new file mode 100644
>> index 0000000..59589be
>> --- /dev/null
>> +++ b/drivers/coresight/coresight-etm.c
>> @@ -0,0 +1,2360 @@
>> +/* Copyright (c) 2011-2012, The Linux Foundation. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 and
>> + * only version 2 as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/types.h>
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/io.h>
>> +#include <linux/err.h>
>> +#include <linux/fs.h>
>> +#include <linux/slab.h>
>> +#include <linux/delay.h>
>> +#include <linux/smp.h>
>> +#include <linux/sysfs.h>
>> +#include <linux/stat.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/clk.h>
>> +#include <linux/cpu.h>
>> +#include <linux/of.h>
>> +#include <linux/of_coresight.h>
>> +#include <linux/coresight.h>
>> +#include <asm/sections.h>
>> +
>> +#include "coresight-priv.h"
>> +
>> +#define etm_writel_mm(drvdata, val, off) \
>> + __raw_writel((val), drvdata->base + off)
>> +#define etm_readl_mm(drvdata, off) \
>> + __raw_readl(drvdata->base + off)
>> +
>> +#define etm_writel(drvdata, val, off) \
>> +({ \
>> + if (drvdata->use_cp14) \
>> + etm_writel_cp14(val, off); \
>> + else \
>> + etm_writel_mm(drvdata, val, off); \
>> +})
>> +#define etm_readl(drvdata, off) \
>> +({ \
>> + uint32_t val; \
>> + if (drvdata->use_cp14) \
>> + val = etm_readl_cp14(off); \
>> + else \
>> + val = etm_readl_mm(drvdata, off); \
>> + val; \
>> +})
>
> Why macros rather than inlines?
Linus W. had the same comment - ACK.
>
>
>> +
>> +#define ETM_LOCK(drvdata) \
>> +do { \
>> + /* Recommended by spec to ensure ETM writes are committed */ \
>> + /* prior to resuming execution */ \
>> + mb(); \
>> + isb(); \
>> + etm_writel_mm(drvdata, 0x0, CORESIGHT_LAR); \
>> +} while (0)
>> +#define ETM_UNLOCK(drvdata) \
>> +do { \
>> + etm_writel_mm(drvdata, CORESIGHT_UNLOCK, CORESIGHT_LAR); \
>> + /* Ensure unlock and any pending writes are committed prior */ \
>> + /* to programming ETM registers */ \
>> + mb(); \
>> + isb(); \
>> +} while (0)
>
> Why macros rather than inlines?
>
>
>> +
>> +#define PORT_SIZE_MASK (BM(21, 21) | BM(4, 6))
>> +
>> +/*
>> + * Device registers:
>> + * 0x000 - 0x2FC: Trace registers
>> + * 0x300 - 0x314: Management registers
>> + * 0x318 - 0xEFC: Trace registers
>> + *
>> + * Coresight registers
>> + * 0xF00 - 0xF9C: Management registers
>> + * 0xFA0 - 0xFA4: Management registers in PFTv1.0
>> + * Trace registers in PFTv1.1
>> + * 0xFA8 - 0xFFC: Management registers
>> + */
>> +
>> +/* Trace registers (0x000-0x2FC) */
>> +#define ETMCR (0x000)
>> +#define ETMCCR (0x004)
>> +#define ETMTRIGGER (0x008)
>> +#define ETMSR (0x010)
>> +#define ETMSCR (0x014)
>> +#define ETMTSSCR (0x018)
>> +#define ETMTECR2 (0x01c)
>> +#define ETMTEEVR (0x020)
>> +#define ETMTECR1 (0x024)
>> +#define ETMFFLR (0x02C)
>> +#define ETMACVRn(n) (0x040 + (n * 4))
>> +#define ETMACTRn(n) (0x080 + (n * 4))
>> +#define ETMCNTRLDVRn(n) (0x140 + (n * 4))
>> +#define ETMCNTENRn(n) (0x150 + (n * 4))
>> +#define ETMCNTRLDEVRn(n) (0x160 + (n * 4))
>> +#define ETMCNTVRn(n) (0x170 + (n * 4))
>> +#define ETMSQ12EVR (0x180)
>> +#define ETMSQ21EVR (0x184)
>> +#define ETMSQ23EVR (0x188)
>> +#define ETMSQ31EVR (0x18C)
>> +#define ETMSQ32EVR (0x190)
>> +#define ETMSQ13EVR (0x194)
>> +#define ETMSQR (0x19C)
>> +#define ETMEXTOUTEVRn(n) (0x1A0 + (n * 4))
>> +#define ETMCIDCVRn(n) (0x1B0 + (n * 4))
>> +#define ETMCIDCMR (0x1BC)
>> +#define ETMIMPSPEC0 (0x1C0)
>> +#define ETMIMPSPEC1 (0x1C4)
>> +#define ETMIMPSPEC2 (0x1C8)
>> +#define ETMIMPSPEC3 (0x1CC)
>> +#define ETMIMPSPEC4 (0x1D0)
>> +#define ETMIMPSPEC5 (0x1D4)
>> +#define ETMIMPSPEC6 (0x1D8)
>> +#define ETMIMPSPEC7 (0x1DC)
>> +#define ETMSYNCFR (0x1E0)
>> +#define ETMIDR (0x1E4)
>> +#define ETMCCER (0x1E8)
>> +#define ETMEXTINSELR (0x1EC)
>> +#define ETMTESSEICR (0x1F0)
>> +#define ETMEIBCR (0x1F4)
>> +#define ETMTSEVR (0x1F8)
>> +#define ETMAUXCR (0x1FC)
>> +#define ETMTRACEIDR (0x200)
>> +#define ETMVMIDCVR (0x240)
>> +/* Management registers (0x300-0x314) */
>> +#define ETMOSLAR (0x300)
>> +#define ETMOSLSR (0x304)
>> +#define ETMOSSRR (0x308)
>> +#define ETMPDCR (0x310)
>> +#define ETMPDSR (0x314)
>
> Move to a header file so the CP14 code can use them ;-)
>
>> +
>> +#define ETM_MAX_ADDR_CMP (16)
>> +#define ETM_MAX_CNTR (4)
>> +#define ETM_MAX_CTXID_CMP (3)
>> +
>> +#define ETM_MODE_EXCLUDE BIT(0)
>> +#define ETM_MODE_CYCACC BIT(1)
>> +#define ETM_MODE_STALL BIT(2)
>> +#define ETM_MODE_TIMESTAMP BIT(3)
>> +#define ETM_MODE_CTXID BIT(4)
>> +#define ETM_MODE_ALL (0x1F)
>> +
>> +#define ETM_EVENT_MASK (0x1FFFF)
>> +#define ETM_SYNC_MASK (0xFFF)
>> +#define ETM_ALL_MASK (0xFFFFFFFF)
>> +
>> +#define ETM_SEQ_STATE_MAX_VAL (0x2)
>> +
>> +enum etm_addr_type {
>> + ETM_ADDR_TYPE_NONE,
>> + ETM_ADDR_TYPE_SINGLE,
>> + ETM_ADDR_TYPE_RANGE,
>> + ETM_ADDR_TYPE_START,
>> + ETM_ADDR_TYPE_STOP,
>> +};
>> +
>> +#ifdef CONFIG_CORESIGHT_SOURCE_ETM_DEFAULT_ENABLE
>> +static int boot_enable = 1;
>> +#else
>> +static int boot_enable;
>> +#endif
>> +module_param_named(
>> + boot_enable, boot_enable, int, S_IRUGO
>> +);
>> +
>> +struct etm_drvdata {
>> + void __iomem *base;
>> + struct device *dev;
>> + struct coresight_device *csdev;
>> + struct clk *clk;
>> + spinlock_t spinlock;
>> + int cpu;
>> + int port_size;
>> + uint8_t arch;
>> + bool use_cp14;
>> + bool enable;
>> + bool sticky_enable;
>> + bool boot_enable;
>> + bool os_unlock;
>> + uint8_t nr_addr_cmp;
>> + uint8_t nr_cntr;
>> + uint8_t nr_ext_inp;
>> + uint8_t nr_ext_out;
>> + uint8_t nr_ctxid_cmp;
>> + uint8_t reset;
>> + uint32_t mode;
>> + uint32_t ctrl;
>> + uint32_t trigger_event;
>> + uint32_t startstop_ctrl;
>> + uint32_t enable_event;
>> + uint32_t enable_ctrl1;
>> + uint32_t fifofull_level;
>> + uint8_t addr_idx;
>> + uint32_t addr_val[ETM_MAX_ADDR_CMP];
>> + uint32_t addr_acctype[ETM_MAX_ADDR_CMP];
>> + uint32_t addr_type[ETM_MAX_ADDR_CMP];
>> + uint8_t cntr_idx;
>> + uint32_t cntr_rld_val[ETM_MAX_CNTR];
>> + uint32_t cntr_event[ETM_MAX_CNTR];
>> + uint32_t cntr_rld_event[ETM_MAX_CNTR];
>> + uint32_t cntr_val[ETM_MAX_CNTR];
>> + uint32_t seq_12_event;
>> + uint32_t seq_21_event;
>> + uint32_t seq_23_event;
>> + uint32_t seq_31_event;
>> + uint32_t seq_32_event;
>> + uint32_t seq_13_event;
>> + uint32_t seq_curr_state;
>> + uint8_t ctxid_idx;
>> + uint32_t ctxid_val[ETM_MAX_CTXID_CMP];
>> + uint32_t ctxid_mask;
>> + uint32_t sync_freq;
>> + uint32_t timestamp_event;
>> +};
>> +
>> +static struct etm_drvdata *etmdrvdata[NR_CPUS];
>> +
>> +/*
>> + * Memory mapped writes to clear os lock are not supported on some processors
>> + * and OS lock must be unlocked before any memory mapped access on such
>> + * processors, otherwise memory mapped reads/writes will be invalid.
>> + */
>> +static void etm_os_unlock(void *info)
>> +{
>> + struct etm_drvdata *drvdata = (struct etm_drvdata *)info;
>> + etm_writel(drvdata, 0x0, ETMOSLAR);
>> + isb();
>> +}
>> +
>> +static void etm_set_pwrdwn(struct etm_drvdata *drvdata)
>> +{
>> + uint32_t etmcr;
>> +
>> + /* Ensure pending cp14 accesses complete before setting pwrdwn */
>> + mb();
>> + isb();
>> + etmcr = etm_readl(drvdata, ETMCR);
>> + etmcr |= BIT(0);
>> + etm_writel(drvdata, etmcr, ETMCR);
>> +}
>> +
>> +static void etm_clr_pwrdwn(struct etm_drvdata *drvdata)
>> +{
>> + uint32_t etmcr;
>> +
>> + etmcr = etm_readl(drvdata, ETMCR);
>> + etmcr &= ~BIT(0);
>> + etm_writel(drvdata, etmcr, ETMCR);
>> + /* Ensure pwrup completes before subsequent cp14 accesses */
>> + mb();
>> + isb();
>> +}
>> +
>> +static void etm_set_pwrup(struct etm_drvdata *drvdata)
>> +{
>> + uint32_t etmpdcr;
>> +
>> + etmpdcr = etm_readl_mm(drvdata, ETMPDCR);
>> + etmpdcr |= BIT(3);
>> + etm_writel_mm(drvdata, etmpdcr, ETMPDCR);
>
> Why are register accesses _mm here? They are not in pwrdown.
Designers can mandate that management registers be access via cp14
only (when supported) but looking at the TRM I just noticed that
coprocessor access to ETMPDCR is "unpredictable". I'll fix that right
away.
>
>
>> + /* Ensure pwrup completes before subsequent cp14 accesses */
>> + mb();
>> + isb();
>> +}
>> +
>> +static void etm_clr_pwrup(struct etm_drvdata *drvdata)
>> +{
>> + uint32_t etmpdcr;
>> +
>> + /* Ensure pending cp14 accesses complete before clearing pwrup */
>> + mb();
>> + isb();
>> + etmpdcr = etm_readl_mm(drvdata, ETMPDCR);
>> + etmpdcr &= ~BIT(3);
>> + etm_writel_mm(drvdata, etmpdcr, ETMPDCR);
>> +}
>
> Same here. Why _mm?
>
>
>> +static void etm_set_prog(struct etm_drvdata *drvdata)
>> +{
>> + uint32_t etmcr;
>> + int count;
>> +
>> + etmcr = etm_readl(drvdata, ETMCR);
>> + etmcr |= BIT(10);
>> + etm_writel(drvdata, etmcr, ETMCR);
>> + /*
>> + * Recommended by spec for cp14 accesses to ensure etmcr write is
>> + * complete before polling etmsr
>> + */
>> + isb();
>> + for (count = TIMEOUT_US; BVAL(etm_readl(drvdata, ETMSR), 1) != 1
>> + && count > 0; count--)
>> + udelay(1);
>> + WARN(count == 0, "timeout while setting prog bit, ETMSR: %#x\n",
>> + etm_readl(drvdata, ETMSR));
>> +}
>> +
>> +static void etm_clr_prog(struct etm_drvdata *drvdata)
>> +{
>> + uint32_t etmcr;
>> + int count;
>> +
>> + etmcr = etm_readl(drvdata, ETMCR);
>> + etmcr &= ~BIT(10);
>> + etm_writel(drvdata, etmcr, ETMCR);
>> + /*
>> + * Recommended by spec for cp14 accesses to ensure etmcr write is
>> + * complete before polling etmsr
>> + */
>> + isb();
>> + for (count = TIMEOUT_US; BVAL(etm_readl(drvdata, ETMSR), 1) != 0
>> + && count > 0; count--)
>> + udelay(1);
>> + WARN(count == 0, "timeout while clearing prog bit, ETMSR: %#x\n",
>> + etm_readl(drvdata, ETMSR));
>> +}
>> +
>> +static void __etm_enable(void *info)
>> +{
>> + int i;
>> + uint32_t etmcr;
>> + struct etm_drvdata *drvdata = info;
>> +
>> + ETM_UNLOCK(drvdata);
>> +
>> + /* turn engine on */
>> + etm_clr_pwrdwn(drvdata);
>> + /* apply power to trace registers */
>> + etm_set_pwrup(drvdata);
>> + /* make sure all registers are accessible */
>> + etm_os_unlock(drvdata);
>> +
>> + etm_set_prog(drvdata);
>> +
>> + etmcr = etm_readl(drvdata, ETMCR);
>> + etmcr &= (BIT(10) | BIT(0));
>> + etmcr |= drvdata->port_size;
>> + etm_writel(drvdata, drvdata->ctrl | etmcr, ETMCR);
>> + etm_writel(drvdata, drvdata->trigger_event, ETMTRIGGER);
>> + etm_writel(drvdata, drvdata->startstop_ctrl, ETMTSSCR);
>> + etm_writel(drvdata, drvdata->enable_event, ETMTEEVR);
>> + etm_writel(drvdata, drvdata->enable_ctrl1, ETMTECR1);
>> + etm_writel(drvdata, drvdata->fifofull_level, ETMFFLR);
>> + for (i = 0; i < drvdata->nr_addr_cmp; i++) {
>> + etm_writel(drvdata, drvdata->addr_val[i], ETMACVRn(i));
>> + etm_writel(drvdata, drvdata->addr_acctype[i], ETMACTRn(i));
>> + }
>> + for (i = 0; i < drvdata->nr_cntr; i++) {
>> + etm_writel(drvdata, drvdata->cntr_rld_val[i], ETMCNTRLDVRn(i));
>> + etm_writel(drvdata, drvdata->cntr_event[i], ETMCNTENRn(i));
>> + etm_writel(drvdata, drvdata->cntr_rld_event[i],
>> + ETMCNTRLDEVRn(i));
>> + etm_writel(drvdata, drvdata->cntr_val[i], ETMCNTVRn(i));
>> + }
>> + etm_writel(drvdata, drvdata->seq_12_event, ETMSQ12EVR);
>> + etm_writel(drvdata, drvdata->seq_21_event, ETMSQ21EVR);
>> + etm_writel(drvdata, drvdata->seq_23_event, ETMSQ23EVR);
>> + etm_writel(drvdata, drvdata->seq_31_event, ETMSQ31EVR);
>> + etm_writel(drvdata, drvdata->seq_32_event, ETMSQ32EVR);
>> + etm_writel(drvdata, drvdata->seq_13_event, ETMSQ13EVR);
>> + etm_writel(drvdata, drvdata->seq_curr_state, ETMSQR);
>> + for (i = 0; i < drvdata->nr_ext_out; i++)
>> + etm_writel(drvdata, 0x0000406F, ETMEXTOUTEVRn(i));
>> + for (i = 0; i < drvdata->nr_ctxid_cmp; i++)
>> + etm_writel(drvdata, drvdata->ctxid_val[i], ETMCIDCVRn(i));
>> + etm_writel(drvdata, drvdata->ctxid_mask, ETMCIDCMR);
>> + etm_writel(drvdata, drvdata->sync_freq, ETMSYNCFR);
>> + etm_writel(drvdata, 0x00000000, ETMEXTINSELR);
>> + etm_writel(drvdata, drvdata->timestamp_event, ETMTSEVR);
>> + etm_writel(drvdata, 0x00000000, ETMAUXCR);
>> + etm_writel(drvdata, drvdata->cpu + 1, ETMTRACEIDR);
>> + etm_writel(drvdata, 0x00000000, ETMVMIDCVR);
>> +
>> + /* ensures trace output is enabled from this ETM */
>> + etm_writel(drvdata, drvdata->ctrl | BIT(11) | etmcr, ETMCR);
>> +
>> + etm_clr_prog(drvdata);
>> + ETM_LOCK(drvdata);
>> +
>> + dev_dbg(drvdata->dev, "cpu: %d enable smp call done\n", drvdata->cpu);
>> +}
>> +
>> +static int etm_enable(struct coresight_device *csdev)
>> +{
>> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> + int ret;
>> +
>> + ret = clk_prepare_enable(drvdata->clk);
>> + if (ret)
>> + goto err_clk;
>> +
>> + spin_lock(&drvdata->spinlock);
>> +
>> + /*
>> + * Executing __etm_enable on the cpu whose ETM is being enabled
>> + * ensures that register writes occur when cpu is powered.
>> + */
>> + ret = smp_call_function_single(drvdata->cpu, __etm_enable, drvdata, 1);
>> + if (ret)
>> + goto err;
>> + drvdata->enable = true;
>> + drvdata->sticky_enable = true;
>> +
>> + spin_unlock(&drvdata->spinlock);
>> +
>> + dev_info(drvdata->dev, "ETM tracing enabled\n");
>> + return 0;
>> +err:
>> + spin_unlock(&drvdata->spinlock);
>> + clk_disable_unprepare(drvdata->clk);
>> +err_clk:
>> + return ret;
>> +}
>> +
>> +static void __etm_disable(void *info)
>> +{
>> + struct etm_drvdata *drvdata = info;
>> +
>> + ETM_UNLOCK(drvdata);
>> + etm_set_prog(drvdata);
>> +
>> + /* Program trace enable to low by using always false event */
>> + etm_writel(drvdata, 0x6F | BIT(14), ETMTEEVR);
>> +
>> + etm_set_pwrdwn(drvdata);
>> + ETM_LOCK(drvdata);
>> +
>> + dev_dbg(drvdata->dev, "cpu: %d disable smp call done\n", drvdata->cpu);
>> +}
>> +
>> +static void etm_disable(struct coresight_device *csdev)
>> +{
>> + struct etm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> + /*
>> + * Taking hotplug lock here protects from clocks getting disabled
>> + * with tracing being left on (crash scenario) if user disable occurs
>> + * after cpu online mask indicates the cpu is offline but before the
>> + * DYING hotplug callback is serviced by the ETM driver.
>> + */
>> + get_online_cpus();
>> + spin_lock(&drvdata->spinlock);
>> +
>> + /*
>> + * Executing __etm_disable on the cpu whose ETM is being disabled
>> + * ensures that register writes occur when cpu is powered.
>> + */
>> + smp_call_function_single(drvdata->cpu, __etm_disable, drvdata, 1);
>> + drvdata->enable = false;
>> +
>> + spin_unlock(&drvdata->spinlock);
>> + put_online_cpus();
>> +
>> + clk_disable_unprepare(drvdata->clk);
>> +
>> + dev_info(drvdata->dev, "ETM tracing disabled\n");
>> +}
>> +
>> +static const struct coresight_ops_source etm_source_ops = {
>> + .enable = etm_enable,
>> + .disable = etm_disable,
>> +};
>> +
>> +static const struct coresight_ops etm_cs_ops = {
>> + .source_ops = &etm_source_ops,
>> +};
>> +
>> +static ssize_t debugfs_show_nr_addr_cmp(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + val = drvdata->nr_addr_cmp;
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static const struct file_operations debugfs_nr_addr_cmp_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_nr_addr_cmp,
>> +};
>
>
> DEFINE_SIMPLE_ATTRIBUTE() would acheive the above with smaller code size
> and no bugs.
I was actually debating doing something like this before or after the
initial RFC. I agree, the syntax can be lightened.
>
>
>> +static const struct coresight_ops_entry debugfs_nr_addr_cmp_entry = {
>> + .name = "nr_addr_cmp",
>> + .mode = S_IRUGO,
>> + .ops = &debugfs_nr_addr_cmp_ops,
>> +};
>
> This (and its friends futher down) look samey enough to merit a
> DEFINE_CORESIGHT_ENTRY() macro.
>
>
>> +static ssize_t debugfs_show_nr_cntr(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + val = drvdata->nr_cntr;
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static const struct file_operations debugfs_nr_cntr_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_nr_cntr,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_nr_cntr_entry = {
>> + .name = "nr_cntr",
>> + .mode = S_IRUGO,
>> + .ops = &debugfs_nr_cntr_ops,
>> +};
>
> DEFINE_SIMPLE_ATTRIBITE() and DEFINE_CORESIGHT_ENTRY()
>
>> +
>> +static ssize_t debugfs_show_nr_ctxid_cmp(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + val = drvdata->nr_ctxid_cmp;
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static const struct file_operations debugfs_nr_ctxid_cmp_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_nr_ctxid_cmp,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_nr_ctxid_cmp_entry = {
>> + .name = "nr_ctxid_cmp",
>> + .mode = S_IRUGO,
>> + .ops = &debugfs_nr_ctxid_cmp_ops,
>> +};
>
> DEFINE_SIMPLE_ATTRIBITE() and DEFINE_CORESIGHT_ENTRY()
>
>> +static ssize_t debugfs_show_reset(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + val = drvdata->reset;
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +/* Reset to trace everything i.e. exclude nothing. */
>> +static ssize_t debugfs_store_reset(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int i;
>> + unsigned long val;
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + if (sscanf(user_buf, "%lx", &val) != 1)
>> + return -EINVAL;
>> +
>> + spin_lock(&drvdata->spinlock);
>> + if (val) {
>> + drvdata->mode = ETM_MODE_EXCLUDE;
>> + drvdata->ctrl = 0x0;
>> + drvdata->trigger_event = 0x406F;
>> + drvdata->startstop_ctrl = 0x0;
>> + drvdata->enable_event = 0x6F;
>> + drvdata->enable_ctrl1 = 0x1000000;
>> + drvdata->fifofull_level = 0x28;
>> + drvdata->addr_idx = 0x0;
>> + for (i = 0; i < drvdata->nr_addr_cmp; i++) {
>> + drvdata->addr_val[i] = 0x0;
>> + drvdata->addr_acctype[i] = 0x0;
>> + drvdata->addr_type[i] = ETM_ADDR_TYPE_NONE;
>> + }
>> + drvdata->cntr_idx = 0x0;
>> + for (i = 0; i < drvdata->nr_cntr; i++) {
>> + drvdata->cntr_rld_val[i] = 0x0;
>> + drvdata->cntr_event[i] = 0x406F;
>> + drvdata->cntr_rld_event[i] = 0x406F;
>> + drvdata->cntr_val[i] = 0x0;
>> + }
>> + drvdata->seq_12_event = 0x406F;
>> + drvdata->seq_21_event = 0x406F;
>> + drvdata->seq_23_event = 0x406F;
>> + drvdata->seq_31_event = 0x406F;
>> + drvdata->seq_32_event = 0x406F;
>> + drvdata->seq_13_event = 0x406F;
>> + drvdata->seq_curr_state = 0x0;
>> + drvdata->ctxid_idx = 0x0;
>> + for (i = 0; i < drvdata->nr_ctxid_cmp; i++)
>> + drvdata->ctxid_val[i] = 0x0;
>> + drvdata->ctxid_mask = 0x0;
>> + drvdata->sync_freq = 0x100;
>> + drvdata->timestamp_event = 0x406F;
>> + }
>> + spin_unlock(&drvdata->spinlock);
>> + return count;
>> +}
>
> This smells like it shared lots of code with __etm_enable(). Not sure
> though with all those 0x406F.
>
> Whatever the case, this code should probably be hoisted out of the
> debugfs code and into a named function.
>
>> +
>> +static const struct file_operations debugfs_reset_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_reset,
>> + .write = debugfs_store_reset,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_reset_entry = {
>> + .name = "reset",
>> + .mode = S_IRUGO | S_IWUSR,
>> + .ops = &debugfs_reset_ops,
>> +};
>> +
>> +static ssize_t debugfs_show_mode(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + val = drvdata->mode;
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static ssize_t debugfs_store_mode(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + unsigned long val;
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + if (sscanf(user_buf, "%lx", &val) != 1)
>> + return -EINVAL;
>> +
>> + spin_lock(&drvdata->spinlock);
>> + drvdata->mode = val & ETM_MODE_ALL;
>> +
>> + if (drvdata->mode & ETM_MODE_EXCLUDE)
>> + drvdata->enable_ctrl1 |= BIT(24);
>> + else
>> + drvdata->enable_ctrl1 &= ~BIT(24);
>> +
>> + if (drvdata->mode & ETM_MODE_CYCACC)
>> + drvdata->ctrl |= BIT(12);
>> + else
>> + drvdata->ctrl &= ~BIT(12);
>> +
>> + if (drvdata->mode & ETM_MODE_STALL)
>> + drvdata->ctrl |= BIT(7);
>> + else
>> + drvdata->ctrl &= ~BIT(7);
>> +
>> + if (drvdata->mode & ETM_MODE_TIMESTAMP)
>> + drvdata->ctrl |= BIT(28);
>> + else
>> + drvdata->ctrl &= ~BIT(28);
>> +
>> + if (drvdata->mode & ETM_MODE_CTXID)
>> + drvdata->ctrl |= (BIT(14) | BIT(15));
>> + else
>> + drvdata->ctrl &= ~(BIT(14) | BIT(15));
>> + spin_unlock(&drvdata->spinlock);
>> +
>> + return count;
>> +}
>> +
>> +static const struct file_operations debugfs_mode_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_mode,
>> + .write = debugfs_store_mode,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_mode_entry = {
>> + .name = "mode",
>> + .mode = S_IRUGO | S_IWUSR,
>> + .ops = &debugfs_mode_ops,
>> +};
>
> DEFINE_SIMPLE_ATTRIBITE() and DEFINE_CORESIGHT_ENTRY()
>
>> +
>> +static ssize_t debugfs_show_trigger_event(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + val = drvdata->trigger_event;
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static ssize_t debugfs_store_trigger_event(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + unsigned long val;
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + if (sscanf(user_buf, "%lx", &val) != 1)
>> + return -EINVAL;
>> +
>> + drvdata->trigger_event = val & ETM_EVENT_MASK;
>> + return count;
>> +}
>> +
>> +static const struct file_operations debugfs_trigger_event_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_trigger_event,
>> + .write = debugfs_store_trigger_event,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_trigger_events_entry = {
>> + .name = "trigger_event",
>> + .mode = S_IRUGO | S_IWUSR,
>> + .ops = &debugfs_trigger_event_ops,
>> +};
>
> DEFINE_SIMPLE_ATTRIBITE() and DEFINE_CORESIGHT_ENTRY()
>
>> +
>> +static ssize_t debugfs_show_enable_event(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + val = drvdata->enable_event;
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static ssize_t debugfs_store_enable_event(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + unsigned long val;
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + if (sscanf(user_buf, "%lx", &val) != 1)
>> + return -EINVAL;
>> +
>> + drvdata->enable_event = val & ETM_EVENT_MASK;
>> + return count;
>> +}
>> +
>> +static const struct file_operations debugfs_enable_event_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_enable_event,
>> + .write = debugfs_store_enable_event,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_enable_events_entry = {
>> + .name = "enable_event",
>> + .mode = S_IRUGO | S_IWUSR,
>> + .ops = &debugfs_enable_event_ops,
>> +};
>> +
>
> DEFINE_SIMPLE_ATTRIBITE() and DEFINE_CORESIGHT_ENTRY()
>
>> +static ssize_t debugfs_show_fifofull_level(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + val = drvdata->fifofull_level;
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static ssize_t debugfs_store_fifofull_level(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + unsigned long val;
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + if (sscanf(user_buf, "%lx", &val) != 1)
>> + return -EINVAL;
>> +
>> + drvdata->fifofull_level = val;
>> + return count;
>> +}
>> +
>> +static const struct file_operations debugfs_fifofull_level_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_fifofull_level,
>> + .write = debugfs_store_fifofull_level,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_fifofull_level_entry = {
>> + .name = "fifofull_level",
>> + .mode = S_IRUGO | S_IWUSR,
>> + .ops = &debugfs_fifofull_level_ops,
>> +};
>> +
>
> DEFINE_SIMPLE_ATTRIBITE() and DEFINE_CORESIGHT_ENTRY()
>
>> +static ssize_t debugfs_show_addr_idx(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + val = drvdata->addr_idx;
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static ssize_t debugfs_store_addr_idx(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + unsigned long val;
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + if (sscanf(user_buf, "%lx", &val) != 1)
>> + return -EINVAL;
>> + if (val >= drvdata->nr_addr_cmp)
>> + return -EINVAL;
>> +
>> + /*
>> + * Use spinlock to ensure index doesn't change while it gets
>> + * dereferenced multiple times within a spinlock block elsewhere.
>> + */
>> + spin_lock(&drvdata->spinlock);
>> + drvdata->addr_idx = val;
>> + spin_unlock(&drvdata->spinlock);
>> + return count;
>> +}
>> +
>> +static const struct file_operations debugfs_addr_idx_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_addr_idx,
>> + .write = debugfs_store_addr_idx,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_addr_idx_entry = {
>> + .name = "addr_idx",
>> + .mode = S_IRUGO | S_IWUSR,
>> + .ops = &debugfs_addr_idx_ops,
>> +};
>> +
>
> DEFINE_SIMPLE_ATTRIBITE() and DEFINE_CORESIGHT_ENTRY()
>
>> +static ssize_t debugfs_show_addr_single(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + uint8_t idx;
>> + unsigned long val;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + spin_lock(&drvdata->spinlock);
>> + idx = drvdata->addr_idx;
>> + if (!(drvdata->addr_type[idx] == ETM_ADDR_TYPE_NONE ||
>> + drvdata->addr_type[idx] == ETM_ADDR_TYPE_SINGLE)) {
>> + spin_unlock(&drvdata->spinlock);
>> + return -EPERM;
>
> -EPERM?
>
> Is this really a permissions check.
That is debatable - you don't have permission because 'addr_type'
doesn't correspond to a single address comparator. "-EINVAL" could
also be returned...
>
>> + }
>> +
>> + val = drvdata->addr_val[idx];
>> + spin_unlock(&drvdata->spinlock);
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static ssize_t debugfs_store_addr_single(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + uint8_t idx;
>> + unsigned long val;
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + if (sscanf(user_buf, "%lx", &val) != 1)
>> + return -EINVAL;
>> +
>> + spin_lock(&drvdata->spinlock);
>> + idx = drvdata->addr_idx;
>> + if (!(drvdata->addr_type[idx] == ETM_ADDR_TYPE_NONE ||
>> + drvdata->addr_type[idx] == ETM_ADDR_TYPE_SINGLE)) {
>> + spin_unlock(&drvdata->spinlock);
>> + return -EPERM;
>
> -EPERM?
>
>> + }
>> +
>> + drvdata->addr_val[idx] = val;
>> + drvdata->addr_type[idx] = ETM_ADDR_TYPE_SINGLE;
>> + spin_unlock(&drvdata->spinlock);
>> + return count;
>> +}
>> +
>> +static const struct file_operations debugfs_addr_single_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_addr_single,
>> + .write = debugfs_store_addr_single,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_addr_single_entry = {
>> + .name = "addr_single",
>> + .mode = S_IRUGO | S_IWUSR,
>> + .ops = &debugfs_addr_single_ops,
>> +};
>
> DEFINE_SIMPLE_ATTRIBITE() and DEFINE_CORESIGHT_ENTRY()
>
>> +static ssize_t debugfs_show_addr_range(struct file *file,
>> + char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + int ret;
>> + uint8_t idx;
>> + unsigned long val1, val2;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + spin_lock(&drvdata->spinlock);
>> + idx = drvdata->addr_idx;
>> + if (idx % 2 != 0) {
>> + spin_unlock(&drvdata->spinlock);
>> + return -EPERM;
>
> -EPERM?
>
>> + }
>> + if (!((drvdata->addr_type[idx] == ETM_ADDR_TYPE_NONE &&
>> + drvdata->addr_type[idx + 1] == ETM_ADDR_TYPE_NONE) ||
>> + (drvdata->addr_type[idx] == ETM_ADDR_TYPE_RANGE &&
>> + drvdata->addr_type[idx + 1] == ETM_ADDR_TYPE_RANGE))) {
>> + spin_unlock(&drvdata->spinlock);
>> + return -EPERM;
>
> -EPERM?
>
>> + }
>> +
>> + val1 = drvdata->addr_val[idx];
>> + val2 = drvdata->addr_val[idx + 1];
>> + spin_unlock(&drvdata->spinlock);
>> + ret = scnprintf(buf, PAGE_SIZE, "%#lx %#lx\n", val1, val2);
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +
>> + kfree(buf);
>> + return ret;
>> +}
>> +
>> +static ssize_t debugfs_store_addr_range(struct file *file,
>> + const char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + uint8_t idx;
>> + unsigned long val1, val2;
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + if (sscanf(user_buf, "%lx %lx", &val1, &val2) != 2)
>> + return -EINVAL;
>> + /* Lower address comparator cannot have a higher address value */
>> + if (val1 > val2)
>> + return -EINVAL;
>> +
>> + spin_lock(&drvdata->spinlock);
>> + idx = drvdata->addr_idx;
>> + if (idx % 2 != 0) {
>> + spin_unlock(&drvdata->spinlock);
>> + return -EPERM;
>
> -EPERM?
>
>> + }
>> + if (!((drvdata->addr_type[idx] == ETM_ADDR_TYPE_NONE &&
>> + drvdata->addr_type[idx + 1] == ETM_ADDR_TYPE_NONE) ||
>> + (drvdata->addr_type[idx] == ETM_ADDR_TYPE_RANGE &&
>> + drvdata->addr_type[idx + 1] == ETM_ADDR_TYPE_RANGE))) {
>> + spin_unlock(&drvdata->spinlock);
>> + return -EPERM;
>
> -EPERM?
>
>> + }
>> +
>> + drvdata->addr_val[idx] = val1;
>> + drvdata->addr_type[idx] = ETM_ADDR_TYPE_RANGE;
>> + drvdata->addr_val[idx + 1] = val2;
>> + drvdata->addr_type[idx + 1] = ETM_ADDR_TYPE_RANGE;
>> + drvdata->enable_ctrl1 |= (1 << (idx/2));
>> + spin_unlock(&drvdata->spinlock);
>> + return count;
>> +}
>> +
>> +static const struct file_operations debugfs_addr_range_ops = {
>> + .open = simple_open,
>> + .read = debugfs_show_addr_range,
>> + .write = debugfs_store_addr_range,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_addr_range_entry = {
>> + .name = "addr_range",
>> + .mode = S_IRUGO | S_IWUSR,
>> + .ops = &debugfs_addr_range_ops,
>> +};
>
> DEFINE_SIMPLE_ATTRIBITE() and DEFINE_CORESIGHT_ENTRY()
>
> SNIP!!!
>
> My comments of debugfs get a bit samey from here on down so I've deleted
> a big chunk.
>
>
>> +static ssize_t debugfs_status_read(struct file *file, char __user *user_buf,
>> + size_t count, loff_t *ppos)
>> +{
>> + ssize_t ret;
>> + uint32_t val;
>> + unsigned long flags;
>> + char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
>> + struct etm_drvdata *drvdata = file->private_data;
>> +
>> + if (!buf)
>> + return -ENOMEM;
>> +
>> + ret = clk_prepare_enable(drvdata->clk);
>> + if (ret)
>> + goto out;
>> +
>> + spin_lock_irqsave(&drvdata->spinlock, flags);
>> +
>> + ETM_UNLOCK(drvdata);
>> + val = etm_readl(drvdata, ETMCCR);
>> + ret += sprintf(buf, "ETMCCR: 0x%08x\n", val);
>> + val = etm_readl(drvdata, ETMCCER);
>> + ret += sprintf(buf + ret, "ETMCCER: 0x%08x\n", val);
>> + val = etm_readl(drvdata, ETMSCR);
>> + ret += sprintf(buf + ret, "ETMSCR: 0x%08x\n", val);
>> + val = etm_readl(drvdata, ETMIDR);
>> + ret += sprintf(buf + ret, "ETMIDR: 0x%08x\n", val);
>> + val = etm_readl(drvdata, ETMCR);
>> + ret += sprintf(buf + ret, "ETMCR: 0x%08x\n", val);
>> + val = etm_readl(drvdata, ETMTEEVR);
>> + ret += sprintf(buf + ret, "Enable event: 0x%08x\n", val);
>> + val = etm_readl(drvdata, ETMTSSCR);
>> + ret += sprintf(buf + ret, "Enable start/stop: 0x%08x\n", val);
>> + ret += sprintf(buf + ret,
>> + "Enable control: CR1 0x%08x CR2 0x%08x\n",
>> + etm_readl(drvdata, ETMTECR1),
>> + etm_readl(drvdata, ETMTECR2));
>> +
>> + ETM_LOCK(drvdata);
>> +
>> + spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> + clk_disable_unprepare(drvdata->clk);
>> +
>> + ret = simple_read_from_buffer(user_buf, count, ppos, buf, ret);
>> +out:
>> + kfree(buf);
>> + return ret;
>> +}
>
> Really not sure whether this should be in the read method. If we don't
> read the file in one go the spin_lock() we'll not get a cohesive set of
> registers.
I get your point but since there is a possibility (even very remove)
that any of these registers can be changed between the two read
operations, the only reasonable solution I see is to return an error
if (ret > size). What your opinion on that?
>
>
>> +
>> +static const struct file_operations debugfs_status_ops = {
>> + .open = simple_open,
>> + .read = debugfs_status_read,
>> +};
>> +
>> +static const struct coresight_ops_entry debugfs_status_entry = {
>> + .name = "status",
>> + .mode = S_IRUGO,
>> + .ops = &debugfs_status_ops,
>> +};
>> +
>> +static const struct coresight_ops_entry *etm_attr_grps[] = {
>> + &debugfs_nr_addr_cmp_entry,
>> + &debugfs_nr_cntr_entry,
>> + &debugfs_nr_ctxid_cmp_entry,
>> + &debugfs_reset_entry,
>> + &debugfs_mode_entry,
>> + &debugfs_trigger_events_entry,
>> + &debugfs_enable_events_entry,
>> + &debugfs_fifofull_level_entry,
>> + &debugfs_addr_idx_entry,
>> + &debugfs_addr_single_entry,
>> + &debugfs_addr_range_entry,
>> + &debugfs_addr_start_entry,
>> + &debugfs_addr_stop_entry,
>> + &debugfs_addr_acctype_entry,
>> + &debugfs_cntr_idx_entry,
>> + &debugfs_cntr_rld_val_entry,
>> + &debugfs_cntr_event_entry,
>> + &debugfs_cntr_rld_event_entry,
>> + &debugfs_cntr_val_entry,
>> + &debugfs_12_event_entry,
>> + &debugfs_21_event_entry,
>> + &debugfs_23_event_entry,
>> + &debugfs_31_event_entry,
>> + &debugfs_32_event_entry,
>> + &debugfs_13_event_entry,
>> + &debugfs_seq_curr_state_entry,
>> + &debugfs_ctxid_idx_entry,
>> + &debugfs_ctxid_val_entry,
>> + &debugfs_ctxid_mask_entry,
>> + &debugfs_sync_freq_entry,
>> + &debugfs_timestamp_event_entry,
>> + &debugfs_status_entry,
>> + NULL,
>> +};
>> +
>> +static int etm_cpu_callback(struct notifier_block *nfb, unsigned long action,
>> + void *hcpu)
>> +{
>> + unsigned int cpu = (unsigned long)hcpu;
>> +
>> + if (!etmdrvdata[cpu])
>> + goto out;
>> +
>> + switch (action & (~CPU_TASKS_FROZEN)) {
>> + case CPU_STARTING:
>> + spin_lock(&etmdrvdata[cpu]->spinlock);
>> + if (!etmdrvdata[cpu]->os_unlock) {
>> + etm_os_unlock(etmdrvdata[cpu]);
>> + etmdrvdata[cpu]->os_unlock = true;
>> + }
>> +
>> + if (etmdrvdata[cpu]->enable)
>> + __etm_enable(etmdrvdata[cpu]);
>> + spin_unlock(&etmdrvdata[cpu]->spinlock);
>> + break;
>> +
>> + case CPU_ONLINE:
>> + if (etmdrvdata[cpu]->boot_enable &&
>> + !etmdrvdata[cpu]->sticky_enable)
>> + coresight_enable(etmdrvdata[cpu]->csdev);
>> + break;
>> +
>> + case CPU_DYING:
>> + spin_lock(&etmdrvdata[cpu]->spinlock);
>> + if (etmdrvdata[cpu]->enable)
>> + __etm_disable(etmdrvdata[cpu]);
>> + spin_unlock(&etmdrvdata[cpu]->spinlock);
>> + break;
>> + }
>> +out:
>> + return NOTIFY_OK;
>> +}
>> +
>> +static struct notifier_block etm_cpu_notifier = {
>> + .notifier_call = etm_cpu_callback,
>> +};
>> +
>> +static bool etm_arch_supported(uint8_t arch)
>> +{
>> + switch (arch) {
>> + case ETM_ARCH_V3_3:
>> + break;
>> + case ETM_ARCH_V3_5:
>> + break;
>> + case PFT_ARCH_V1_1:
>> + break;
>> + default:
>> + return false;
>> + }
>> + return true;
>> +}
>> +
>> +static void etm_init_arch_data(void *info)
>> +{
>> + uint32_t etmidr;
>> + uint32_t etmccr;
>> + struct etm_drvdata *drvdata = info;
>> +
>> + ETM_UNLOCK(drvdata);
>> +
>> + /* first dummy read */
>> + (void)etm_readl(drvdata, ETMPDSR);
>> + /* Provide power to ETM: ETMPDCR[3] == 1 */
>> + etm_set_pwrup(drvdata);
>> + /*
>> + * Clear power down bit since when this bit is set writes to
>> + * certain registers might be ignored.
>> + */
>> + etm_clr_pwrdwn(drvdata);
>> + /*
>> + * Set prog bit. It will be set from reset but this is included to
>> + * ensure it is set
>> + */
>> + etm_set_prog(drvdata);
>> +
>> + /* Find all capabilities */
>> + etmidr = etm_readl(drvdata, ETMIDR);
>> + drvdata->arch = BMVAL(etmidr, 4, 11);
>> + drvdata->port_size = etm_readl(drvdata, ETMCR) & PORT_SIZE_MASK;
>> +
>> + etmccr = etm_readl(drvdata, ETMCCR);
>> + drvdata->nr_addr_cmp = BMVAL(etmccr, 0, 3) * 2;
>> + drvdata->nr_cntr = BMVAL(etmccr, 13, 15);
>> + drvdata->nr_ext_inp = BMVAL(etmccr, 17, 19);
>> + drvdata->nr_ext_out = BMVAL(etmccr, 20, 22);
>> + drvdata->nr_ctxid_cmp = BMVAL(etmccr, 24, 25);
>> +
>> + etm_set_pwrdwn(drvdata);
>> + etm_clr_pwrup(drvdata);
>> + ETM_LOCK(drvdata);
>> +}
>> +
>> +static void etm_init_default_data(struct etm_drvdata *drvdata)
>> +{
>> + int i;
>> +
>> + uint32_t flags = (1 << 0 | /* instruction execute*/
>> + 3 << 3 | /* ARM instruction */
>> + 0 << 5 | /* No data value comparison */
>> + 0 << 7 | /* No exact mach */
>> + 0 << 8 | /* Ignore context ID */
>> + 0 << 10); /* Security ignored */
>> +
>> + drvdata->ctrl = (BIT(12) | /* cycle accurate */
>> + BIT(28)); /* timestamp */
>> + drvdata->trigger_event = 0x406F;
>> + drvdata->enable_event = 0x6F;
>> + drvdata->enable_ctrl1 = 0x1;
>> + drvdata->fifofull_level = 0x28;
>> + if (drvdata->nr_addr_cmp >= 2) {
>> + drvdata->addr_val[0] = (uint32_t) _stext;
>> + drvdata->addr_val[1] = (uint32_t) _etext;
>> + drvdata->addr_acctype[0] = flags;
>> + drvdata->addr_acctype[1] = flags;
>> + drvdata->addr_type[0] = ETM_ADDR_TYPE_RANGE;
>> + drvdata->addr_type[1] = ETM_ADDR_TYPE_RANGE;
>> + }
>> + for (i = 0; i < drvdata->nr_cntr; i++) {
>> + drvdata->cntr_event[i] = 0x406F;
>> + drvdata->cntr_rld_event[i] = 0x406F;
>> + }
>> + drvdata->seq_12_event = 0x406F;
>> + drvdata->seq_21_event = 0x406F;
>> + drvdata->seq_23_event = 0x406F;
>> + drvdata->seq_31_event = 0x406F;
>> + drvdata->seq_32_event = 0x406F;
>> + drvdata->seq_13_event = 0x406F;
>> + drvdata->sync_freq = 0x100;
>> + drvdata->timestamp_event = 0x406F;
>> +}
>
> Ah... here's all those 0x406F I thought looked odd in the implementation
> of the reset attribute. This code should be commoned up as much as
> possible with the reset code.
>
> Also perhaps a #define to explain what 0x406F means?
>
>
>> +
>> +static int etm_probe(struct platform_device *pdev)
>> +{
>> + int ret;
>> + struct device *dev = &pdev->dev;
>> + struct coresight_platform_data *pdata = NULL;
>> + struct etm_drvdata *drvdata;
>> + struct resource *res;
>> + static int count;
>
> That "static" is very well concealed. I missed that until I started
> studying the error paths.
>
>> + struct coresight_desc *desc;
>> +
>> + drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> + if (!drvdata)
>> + return -ENOMEM;
>> +
>> + if (pdev->dev.of_node) {
>> + pdata = of_get_coresight_platform_data(dev, pdev->dev.of_node);
>> + if (IS_ERR(pdata))
>> + return PTR_ERR(pdata);
>> + pdev->dev.platform_data = pdata;
>> + drvdata->use_cp14 = of_property_read_bool(pdev->dev.of_node,
>> + "arm,cp14");
>> + }
>> +
>> + drvdata->dev = &pdev->dev;
>> + platform_set_drvdata(pdev, drvdata);
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res)
>> + return -ENODEV;
>> +
>> + drvdata->base = devm_ioremap(dev, res->start, resource_size(res));
>
> Leak on error paths?
Yes definitely - I'll fix that.
>
>> + if (!drvdata->base)
>> + return -ENOMEM;
>> +
>> + spin_lock_init(&drvdata->spinlock);
>> +
>> + if (pdata && pdata->clk) {
>> + drvdata->clk = pdata->clk;
>> + ret = clk_prepare_enable(drvdata->clk);
>> + if (ret)
>> + return ret;
>> + }
>> +
>> + drvdata->cpu = pdata ? pdata->cpu : 0;
>> +
>> + get_online_cpus();
>> + etmdrvdata[drvdata->cpu] = drvdata;
>> +
>> + if (!smp_call_function_single(drvdata->cpu, etm_os_unlock, drvdata, 1))
>> + drvdata->os_unlock = true;
>> +
>> + if (smp_call_function_single(drvdata->cpu,
>> + etm_init_arch_data, drvdata, 1))
>> + dev_err(dev, "ETM arch init failed\n");
>> +
>> + if (!count++)
>
> count is mishandled on the error paths?
Humm... I see your point - if this is the ETM that registers the
notifier and "etm_arch_supported()" below fails, we have a dead
notifier. I'll fix that.
>
>> + register_hotcpu_notifier(&etm_cpu_notifier);
>
> Leak on (some of the) error paths?
>
>> +
>> + put_online_cpus();
>> +
>> + if (etm_arch_supported(drvdata->arch) == false) {
>> + clk_disable_unprepare(drvdata->clk);
>> + return -EINVAL;
>> + }
>> + etm_init_default_data(drvdata);
>> +
>> + clk_disable_unprepare(drvdata->clk);
>> +
>> + desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
>
> Leak on error paths?
>
>> + if (!desc) {
>> + ret = -ENOMEM;
>> + goto err;
>> + }
>> + desc->type = CORESIGHT_DEV_TYPE_SOURCE;
>> + desc->subtype.source_subtype = CORESIGHT_DEV_SUBTYPE_SOURCE_PROC;
>> + desc->ops = &etm_cs_ops;
>> + desc->pdata = pdev->dev.platform_data;
>> + desc->dev = &pdev->dev;
>> + desc->debugfs_ops = etm_attr_grps;
>> + desc->owner = THIS_MODULE;
>> + drvdata->csdev = coresight_register(desc);
>> + if (IS_ERR(drvdata->csdev)) {
>> + ret = PTR_ERR(drvdata->csdev);
>> + goto err;
>> + }
>> +
>> + dev_info(dev, "ETM initialized\n");
>> +
>> + if (boot_enable) {
>> + coresight_enable(drvdata->csdev);
>> + drvdata->boot_enable = true;
>> + }
>> +
>> + return 0;
>> +err:
>> + if (drvdata->cpu == 0)
>> + unregister_hotcpu_notifier(&etm_cpu_notifier);
>> + return ret;
>> +}
>> +
>> +static int etm_remove(struct platform_device *pdev)
>> +{
>> + struct etm_drvdata *drvdata = platform_get_drvdata(pdev);
>> +
>> + coresight_unregister(drvdata->csdev);
>> + if (drvdata->cpu == 0)
>> + unregister_hotcpu_notifier(&etm_cpu_notifier);
>> + return 0;
>> +}
>> +
>> +static struct of_device_id etm_match[] = {
>> + {.compatible = "arm,coresight-etm"},
>> + {}
>> +};
>> +
>> +static struct platform_driver etm_driver = {
>> + .probe = etm_probe,
>> + .remove = etm_remove,
>> + .driver = {
>> + .name = "coresight-etm",
>> + .owner = THIS_MODULE,
>> + .of_match_table = etm_match,
>> + },
>> +};
>> +
>> +int __init etm_init(void)
>> +{
>> + return platform_driver_register(&etm_driver);
>> +}
>> +module_init(etm_init);
>> +
>> +void __exit etm_exit(void)
>> +{
>> + platform_driver_unregister(&etm_driver);
>> +}
>> +module_exit(etm_exit);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_DESCRIPTION("CoreSight Program Flow Trace driver");
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists