Message ID | 1460483692-25061-5-git-send-email-mathieu.poirier@linaro.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 12/04/16 18:54, Mathieu Poirier wrote: > The amount of #define, enumeration and structure definition > is big enough to justify moving them to a new header file. > > +/* TMC_STS - 0x00C */ > +#define TMC_STS_TRIGGERED BIT(1) ... > +#define TMC_AXICTL_WR_BURST_LEN 0xF00 Nit: The value above signifies, 16 data transfers per burst. So ideally it would be good to rename it to reflect that. say, TMC_AXICTL_WR_BURST_16 > +/* TMC_FFCR - 0x304 */ > +#define TMC_FFCR_EN_FMT BIT(0) > +#define TMC_FFCR_EN_TI BIT(1) > +#define TMC_FFCR_FON_FLIN BIT(4) > +#define TMC_FFCR_FON_TRIG_EVT BIT(5) > +#define TMC_FFCR_FLUSHMAN BIT(6) > +#define TMC_FFCR_TRIGON_TRIGIN BIT(8) > +#define TMC_FFCR_STOP_ON_FLUSH BIT(12) > + > +#define TMC_STS_TMCREADY_BIT 2 > +#define TMC_FFCR_FLUSHMAN_BIT 6 nit: It would be nice to group the STS_ and FFCR_ bits together. Also I see that the defintion for TMC_STS_FULL is added in a completely unrelated patch (TMC-ETF AUX SPACE patch ?). It would be good to add it either here or in a different patch. Otherwise looks good. Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> Thanks Suzuki
On 14 April 2016 at 11:33, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > On 12/04/16 18:54, Mathieu Poirier wrote: >> >> The amount of #define, enumeration and structure definition >> is big enough to justify moving them to a new header file. >> > > >> +/* TMC_STS - 0x00C */ >> +#define TMC_STS_TRIGGERED BIT(1) > > > ... > >> +#define TMC_AXICTL_WR_BURST_LEN 0xF00 > > > Nit: The value above signifies, 16 data transfers per burst. > So ideally it would be good to rename it to reflect that. say, > > TMC_AXICTL_WR_BURST_16 Will do. But I'll have to do this in a separate patch then the grouping of STS_ and FFCR_ defines you're referring to below since it will also require changes to the .c files. > > > >> +/* TMC_FFCR - 0x304 */ >> +#define TMC_FFCR_EN_FMT BIT(0) >> +#define TMC_FFCR_EN_TI BIT(1) >> +#define TMC_FFCR_FON_FLIN BIT(4) >> +#define TMC_FFCR_FON_TRIG_EVT BIT(5) >> +#define TMC_FFCR_FLUSHMAN BIT(6) >> +#define TMC_FFCR_TRIGON_TRIGIN BIT(8) >> +#define TMC_FFCR_STOP_ON_FLUSH BIT(12) >> + >> +#define TMC_STS_TMCREADY_BIT 2 > > >> +#define TMC_FFCR_FLUSHMAN_BIT 6 > > > nit: It would be nice to group the STS_ and FFCR_ bits together. > Also I see that the defintion for > TMC_STS_FULL is added in a completely unrelated patch (TMC-ETF AUX SPACE > patch ?). It would be good to add it either here or in a different patch. TMC_STS_FULL is not added here because at this point it is not used by the code - it is only added later when it is useful. Please get back to me if I missed your point. > > Otherwise looks good. > > Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com> > > Thanks > Suzuki
On 15/04/16 17:03, Mathieu Poirier wrote: > On 14 April 2016 at 11:33, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: >> On 12/04/16 18:54, Mathieu Poirier wrote: >>> >>> The amount of #define, enumeration and structure definition >>> is big enough to justify moving them to a new header file. >>> >> >> >>> +/* TMC_STS - 0x00C */ >>> +#define TMC_STS_TRIGGERED BIT(1) >> >> >> ... >> >>> +#define TMC_AXICTL_WR_BURST_LEN 0xF00 >> >> >> Nit: The value above signifies, 16 data transfers per burst. >> So ideally it would be good to rename it to reflect that. say, >> >> TMC_AXICTL_WR_BURST_16 > > Will do. But I'll have to do this in a separate patch then the > grouping of STS_ and FFCR_ defines you're referring to below since it > will also require changes to the .c files. Yes, I don't expect this change to be part of the patch. Separate patch is fine. > >> >> >> >>> +/* TMC_FFCR - 0x304 */ >>> +#define TMC_FFCR_EN_FMT BIT(0) >>> +#define TMC_FFCR_EN_TI BIT(1) >>> +#define TMC_FFCR_FON_FLIN BIT(4) >>> +#define TMC_FFCR_FON_TRIG_EVT BIT(5) >>> +#define TMC_FFCR_FLUSHMAN BIT(6) >>> +#define TMC_FFCR_TRIGON_TRIGIN BIT(8) >>> +#define TMC_FFCR_STOP_ON_FLUSH BIT(12) >>> + >>> +#define TMC_STS_TMCREADY_BIT 2 >> >> >>> +#define TMC_FFCR_FLUSHMAN_BIT 6 >> >> >> nit: It would be nice to group the STS_ and FFCR_ bits together. >> Also I see that the defintion for >> TMC_STS_FULL is added in a completely unrelated patch (TMC-ETF AUX SPACE >> patch ?). It would be good to add it either here or in a different patch. > > TMC_STS_FULL is not added here because at this point it is not used by > the code - it is only added later when it is useful. I agree. But the patch which introduces the definition doesn't deal with TMC_STS_ at all either. Thats why I said, either here or in a different patch than what is there. May be you could club the change above and the STS_FULL into a new single patch. Its not mandatory though. Suzuki
On 15 April 2016 at 10:08, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: > On 15/04/16 17:03, Mathieu Poirier wrote: >> >> On 14 April 2016 at 11:33, Suzuki K Poulose <Suzuki.Poulose@arm.com> >> wrote: >>> >>> On 12/04/16 18:54, Mathieu Poirier wrote: >>>> >>>> >>>> The amount of #define, enumeration and structure definition >>>> is big enough to justify moving them to a new header file. >>>> >>> >>> >>>> +/* TMC_STS - 0x00C */ >>>> +#define TMC_STS_TRIGGERED BIT(1) >>> >>> >>> >>> ... >>> >>>> +#define TMC_AXICTL_WR_BURST_LEN 0xF00 >>> >>> >>> >>> Nit: The value above signifies, 16 data transfers per burst. >>> So ideally it would be good to rename it to reflect that. say, >>> >>> TMC_AXICTL_WR_BURST_16 >> >> >> Will do. But I'll have to do this in a separate patch then the >> grouping of STS_ and FFCR_ defines you're referring to below since it >> will also require changes to the .c files. > > > Yes, I don't expect this change to be part of the patch. Separate patch > is fine. > >> >>> >>> >>> >>>> +/* TMC_FFCR - 0x304 */ >>>> +#define TMC_FFCR_EN_FMT BIT(0) >>>> +#define TMC_FFCR_EN_TI BIT(1) >>>> +#define TMC_FFCR_FON_FLIN BIT(4) >>>> +#define TMC_FFCR_FON_TRIG_EVT BIT(5) >>>> +#define TMC_FFCR_FLUSHMAN BIT(6) >>>> +#define TMC_FFCR_TRIGON_TRIGIN BIT(8) >>>> +#define TMC_FFCR_STOP_ON_FLUSH BIT(12) >>>> + >>>> +#define TMC_STS_TMCREADY_BIT 2 >>> >>> >>> >>>> +#define TMC_FFCR_FLUSHMAN_BIT 6 >>> >>> >>> >>> nit: It would be nice to group the STS_ and FFCR_ bits together. >>> Also I see that the defintion for >>> TMC_STS_FULL is added in a completely unrelated patch (TMC-ETF AUX SPACE >>> patch ?). It would be good to add it either here or in a different patch. >> >> >> TMC_STS_FULL is not added here because at this point it is not used by >> the code - it is only added later when it is useful. > > > I agree. But the patch which introduces the definition doesn't deal with > TMC_STS_ at all either. Patch 13/15 is using the TMC_STS_FULL define on line 147. Am I missing your point? >Thats why I said, either here or in a different > patch > than what is there. May be you could club the change above and the STS_FULL > into a new single patch. Its not mandatory though. > > Suzuki
On 15/04/16 17:15, Mathieu Poirier wrote: > On 15 April 2016 at 10:08, Suzuki K Poulose <Suzuki.Poulose@arm.com> wrote: >> On 15/04/16 17:03, Mathieu Poirier wrote: >> I agree. But the patch which introduces the definition doesn't deal with >> TMC_STS_ at all either. > > Patch 13/15 is using the TMC_STS_FULL define on line 147. Am I > missing your point? Ah, sorry, didn't see that TMC_STS_FULL was used there. Ignore my comments.
diff --git a/drivers/hwtracing/coresight/coresight-tmc.c b/drivers/hwtracing/coresight/coresight-tmc.c index d211aeec49f8..01a7ce2974f7 100644 --- a/drivers/hwtracing/coresight/coresight-tmc.c +++ b/drivers/hwtracing/coresight/coresight-tmc.c @@ -30,107 +30,7 @@ #include <linux/amba/bus.h> #include "coresight-priv.h" - -#define TMC_RSZ 0x004 -#define TMC_STS 0x00c -#define TMC_RRD 0x010 -#define TMC_RRP 0x014 -#define TMC_RWP 0x018 -#define TMC_TRG 0x01c -#define TMC_CTL 0x020 -#define TMC_RWD 0x024 -#define TMC_MODE 0x028 -#define TMC_LBUFLEVEL 0x02c -#define TMC_CBUFLEVEL 0x030 -#define TMC_BUFWM 0x034 -#define TMC_RRPHI 0x038 -#define TMC_RWPHI 0x03c -#define TMC_AXICTL 0x110 -#define TMC_DBALO 0x118 -#define TMC_DBAHI 0x11c -#define TMC_FFSR 0x300 -#define TMC_FFCR 0x304 -#define TMC_PSCR 0x308 -#define TMC_ITMISCOP0 0xee0 -#define TMC_ITTRFLIN 0xee8 -#define TMC_ITATBDATA0 0xeec -#define TMC_ITATBCTR2 0xef0 -#define TMC_ITATBCTR1 0xef4 -#define TMC_ITATBCTR0 0xef8 - -/* register description */ -/* TMC_CTL - 0x020 */ -#define TMC_CTL_CAPT_EN BIT(0) -/* TMC_STS - 0x00C */ -#define TMC_STS_TRIGGERED BIT(1) -/* TMC_AXICTL - 0x110 */ -#define TMC_AXICTL_PROT_CTL_B0 BIT(0) -#define TMC_AXICTL_PROT_CTL_B1 BIT(1) -#define TMC_AXICTL_SCT_GAT_MODE BIT(7) -#define TMC_AXICTL_WR_BURST_LEN 0xF00 -/* TMC_FFCR - 0x304 */ -#define TMC_FFCR_EN_FMT BIT(0) -#define TMC_FFCR_EN_TI BIT(1) -#define TMC_FFCR_FON_FLIN BIT(4) -#define TMC_FFCR_FON_TRIG_EVT BIT(5) -#define TMC_FFCR_FLUSHMAN BIT(6) -#define TMC_FFCR_TRIGON_TRIGIN BIT(8) -#define TMC_FFCR_STOP_ON_FLUSH BIT(12) - -#define TMC_STS_TMCREADY_BIT 2 -#define TMC_FFCR_FLUSHMAN_BIT 6 - -enum tmc_config_type { - TMC_CONFIG_TYPE_ETB, - TMC_CONFIG_TYPE_ETR, - TMC_CONFIG_TYPE_ETF, -}; - -enum tmc_mode { - TMC_MODE_CIRCULAR_BUFFER, - TMC_MODE_SOFTWARE_FIFO, - TMC_MODE_HARDWARE_FIFO, -}; - -enum tmc_mem_intf_width { - TMC_MEM_INTF_WIDTH_32BITS = 0x2, - TMC_MEM_INTF_WIDTH_64BITS = 0x3, - TMC_MEM_INTF_WIDTH_128BITS = 0x4, - TMC_MEM_INTF_WIDTH_256BITS = 0x5, -}; - -/** - * struct tmc_drvdata - specifics associated to an TMC component - * @base: memory mapped base address for this component. - * @dev: the device entity associated to this component. - * @csdev: component vitals needed by the framework. - * @miscdev: specifics to handle "/dev/xyz.tmc" entry. - * @spinlock: only one at a time pls. - * @read_count: manages preparation of buffer for reading. - * @buf: area of memory where trace data get sent. - * @paddr: DMA start location in RAM. - * @vaddr: virtual representation of @paddr. - * @size: @buf size. - * @enable: this TMC is being used. - * @config_type: TMC variant, must be of type @tmc_config_type. - * @trigger_cntr: amount of words to store after a trigger. - */ -struct tmc_drvdata { - void __iomem *base; - struct device *dev; - struct coresight_device *csdev; - struct miscdevice miscdev; - spinlock_t spinlock; - int read_count; - bool reading; - char *buf; - dma_addr_t paddr; - void *vaddr; - u32 size; - bool enable; - enum tmc_config_type config_type; - u32 trigger_cntr; -}; +#include "coresight-tmc.h" static void tmc_wait_for_tmcready(struct tmc_drvdata *drvdata) { diff --git a/drivers/hwtracing/coresight/coresight-tmc.h b/drivers/hwtracing/coresight/coresight-tmc.h new file mode 100644 index 000000000000..2d7d52747b4e --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-tmc.h @@ -0,0 +1,122 @@ +/* + * Copyright(C) 2015 Linaro Limited. All rights reserved. + * Author: Mathieu Poirier <mathieu.poirier@linaro.org> + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License 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. + * + * You should have received a copy of the GNU General Public License along with + * this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef _CORESIGHT_TMC_H +#define _CORESIGHT_TMC_H + +#define TMC_RSZ 0x004 +#define TMC_STS 0x00c +#define TMC_RRD 0x010 +#define TMC_RRP 0x014 +#define TMC_RWP 0x018 +#define TMC_TRG 0x01c +#define TMC_CTL 0x020 +#define TMC_RWD 0x024 +#define TMC_MODE 0x028 +#define TMC_LBUFLEVEL 0x02c +#define TMC_CBUFLEVEL 0x030 +#define TMC_BUFWM 0x034 +#define TMC_RRPHI 0x038 +#define TMC_RWPHI 0x03c +#define TMC_AXICTL 0x110 +#define TMC_DBALO 0x118 +#define TMC_DBAHI 0x11c +#define TMC_FFSR 0x300 +#define TMC_FFCR 0x304 +#define TMC_PSCR 0x308 +#define TMC_ITMISCOP0 0xee0 +#define TMC_ITTRFLIN 0xee8 +#define TMC_ITATBDATA0 0xeec +#define TMC_ITATBCTR2 0xef0 +#define TMC_ITATBCTR1 0xef4 +#define TMC_ITATBCTR0 0xef8 + +/* register description */ +/* TMC_CTL - 0x020 */ +#define TMC_CTL_CAPT_EN BIT(0) +/* TMC_STS - 0x00C */ +#define TMC_STS_TRIGGERED BIT(1) +/* TMC_AXICTL - 0x110 */ +#define TMC_AXICTL_PROT_CTL_B0 BIT(0) +#define TMC_AXICTL_PROT_CTL_B1 BIT(1) +#define TMC_AXICTL_SCT_GAT_MODE BIT(7) +#define TMC_AXICTL_WR_BURST_LEN 0xF00 +/* TMC_FFCR - 0x304 */ +#define TMC_FFCR_EN_FMT BIT(0) +#define TMC_FFCR_EN_TI BIT(1) +#define TMC_FFCR_FON_FLIN BIT(4) +#define TMC_FFCR_FON_TRIG_EVT BIT(5) +#define TMC_FFCR_FLUSHMAN BIT(6) +#define TMC_FFCR_TRIGON_TRIGIN BIT(8) +#define TMC_FFCR_STOP_ON_FLUSH BIT(12) + +#define TMC_STS_TMCREADY_BIT 2 +#define TMC_FFCR_FLUSHMAN_BIT 6 + +enum tmc_config_type { + TMC_CONFIG_TYPE_ETB, + TMC_CONFIG_TYPE_ETR, + TMC_CONFIG_TYPE_ETF, +}; + +enum tmc_mode { + TMC_MODE_CIRCULAR_BUFFER, + TMC_MODE_SOFTWARE_FIFO, + TMC_MODE_HARDWARE_FIFO, +}; + +enum tmc_mem_intf_width { + TMC_MEM_INTF_WIDTH_32BITS = 0x2, + TMC_MEM_INTF_WIDTH_64BITS = 0x3, + TMC_MEM_INTF_WIDTH_128BITS = 0x4, + TMC_MEM_INTF_WIDTH_256BITS = 0x5, +}; + +/** + * struct tmc_drvdata - specifics associated to an TMC component + * @base: memory mapped base address for this component. + * @dev: the device entity associated to this component. + * @csdev: component vitals needed by the framework. + * @miscdev: specifics to handle "/dev/xyz.tmc" entry. + * @spinlock: only one at a time pls. + * @read_count: manages preparation of buffer for reading. + * @buf: area of memory where trace data get sent. + * @paddr: DMA start location in RAM. + * @vaddr: virtual representation of @paddr. + * @size: @buf size. + * @enable: this TMC is being used. + * @config_type: TMC variant, must be of type @tmc_config_type. + * @trigger_cntr: amount of words to store after a trigger. + */ +struct tmc_drvdata { + void __iomem *base; + struct device *dev; + struct coresight_device *csdev; + struct miscdevice miscdev; + spinlock_t spinlock; + int read_count; + bool reading; + char *buf; + dma_addr_t paddr; + void __iomem *vaddr; + u32 size; + bool enable; + enum tmc_config_type config_type; + u32 trigger_cntr; +}; + +#endif
The amount of #define, enumeration and structure definition is big enough to justify moving them to a new header file. Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org> --- drivers/hwtracing/coresight/coresight-tmc.c | 102 +---------------------- drivers/hwtracing/coresight/coresight-tmc.h | 122 ++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+), 101 deletions(-) create mode 100644 drivers/hwtracing/coresight/coresight-tmc.h