diff mbox

[RFC,02/11] coresight: add CoreSight TMC driver

Message ID 1401457391-12242-3-git-send-email-mathieu.poirier@linaro.org
State New
Headers show

Commit Message

Mathieu Poirier May 30, 2014, 1:43 p.m. UTC
From: Pratik Patel <pratikp@codeaurora.org>

This driver manages CoreSight TMC (Trace Memory Controller) which
can act as a link or a sink depending upon its configuration. It
can present itself as an ETF (Embedded Trace FIFO) or ETR
(Embedded Trace Router).

ETF when configured in circular buffer mode acts as a trace
collection sink. When configured in HW fifo mode it acts as link.
ETR always acts as a sink and can be used to route data to memory
allocated in RAM.

Signed-off-by: Pratik Patel <pratikp@codeaurora.org>
Signed-off-by: Panchaxari Prasannamurthy <panchaxari.prasannamurthy@linaro.org>
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/coresight/Kconfig         |  11 +
 drivers/coresight/Makefile        |   1 +
 drivers/coresight/coresight-tmc.c | 796 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 808 insertions(+)
 create mode 100644 drivers/coresight/coresight-tmc.c

Comments

Linus Walleij June 3, 2014, 9:09 a.m. UTC | #1
On Fri, May 30, 2014 at 3:43 PM,  <mathieu.poirier@linaro.org> wrote:

> +#define tmc_writel(drvdata, val, off)  __raw_writel((val), drvdata->base + off)
> +#define tmc_readl(drvdata, off)                __raw_readl(drvdata->base + off)

Why not writel_relaxed()/readl_relaxed()?

Using __raw* accessors seem a bit thick. (Applies to all such defines.)

> +#define TMC_LOCK(drvdata)                                              \
> +do {                                                                   \
> +       /* settle everything first */                                   \
> +       mb();                                                           \
> +       tmc_writel(drvdata, 0x0, CORESIGHT_LAR);                        \
> +} while (0)
> +#define TMC_UNLOCK(drvdata)                                            \
> +do {                                                                   \
> +       tmc_writel(drvdata, CORESIGHT_UNLOCK, CORESIGHT_LAR);           \
> +       /* make sure everyone sees this */                              \
> +       mb();                                                           \
> +} while (0)

Convert these to static inlines. No need for them to be #defines
at all really.

> +#define BYTES_PER_WORD         4

But please. Just using the number 4 everywhere is clear enough.

> +struct tmc_drvdata {
> +       void __iomem            *base;
> +       struct device           *dev;
> +       struct coresight_device *csdev;
> +       struct miscdevice       miscdev;
> +       struct clk              *clk;
> +       spinlock_t              spinlock;
> +       int                     read_count;

Can this really be negative?

> +       bool                    reading;
> +       char                    *buf;
> +       dma_addr_t              paddr;
> +       void __iomem            *vaddr;
> +       uint32_t                size;

Use u32

> +       bool                    enable;
> +       enum tmc_config_type    config_type;
> +       uint32_t                trigger_cntr;

Use u32

> +};

This struct overall could use some kerneldoc.

> +static void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
> +{
> +       int count;

Why not call this variable "i" as per convention.

> +       uint32_t ffcr;

u32

> +
> +       ffcr = tmc_readl(drvdata, TMC_FFCR);
> +       ffcr |= BIT(12);
> +       tmc_writel(drvdata, ffcr, TMC_FFCR);
> +       ffcr |= BIT(6);

A bit unclear what bit 12 and 6 does. Either #define them or add comments.

> +       tmc_writel(drvdata, ffcr, TMC_FFCR);
> +       /* Ensure flush completes */
> +       for (count = TIMEOUT_US; BVAL(tmc_readl(drvdata, TMC_FFCR), 6) != 0
> +                               && count > 0; count--)
> +               udelay(1);
> +       WARN(count == 0, "timeout while flushing TMC, TMC_FFCR: %#x\n",
> +            tmc_readl(drvdata, TMC_FFCR));
> +
> +       tmc_wait_for_ready(drvdata);
> +}
> +
> +static void __tmc_enable(struct tmc_drvdata *drvdata)
> +{
> +       tmc_writel(drvdata, 0x1, TMC_CTL);
> +}
> +
> +static void __tmc_disable(struct tmc_drvdata *drvdata)
> +{
> +       tmc_writel(drvdata, 0x0, TMC_CTL);
> +}

I actually understand what bit 0 does in this register, but could
also be #defined.

> +static void __tmc_etb_enable(struct tmc_drvdata *drvdata)
> +{
> +       /* Zero out the memory to help with debug */
> +       memset(drvdata->buf, 0, drvdata->size);
> +
> +       TMC_UNLOCK(drvdata);
> +
> +       tmc_writel(drvdata, TMC_MODE_CIRCULAR_BUFFER, TMC_MODE);
> +       tmc_writel(drvdata, 0x133, TMC_FFCR);

0x133? Que ce que c'est?

> +       tmc_writel(drvdata, drvdata->trigger_cntr, TMC_TRG);
> +       __tmc_enable(drvdata);
> +
> +       TMC_LOCK(drvdata);
> +}
> +
> +static void __tmc_etr_enable(struct tmc_drvdata *drvdata)
> +{
> +       uint32_t axictl;

u32

> +       /* Zero out the memory to help with debug */
> +       memset(drvdata->vaddr, 0, drvdata->size);
> +
> +       TMC_UNLOCK(drvdata);
> +
> +       tmc_writel(drvdata, drvdata->size / BYTES_PER_WORD, TMC_RSZ);
> +       tmc_writel(drvdata, TMC_MODE_CIRCULAR_BUFFER, TMC_MODE);
> +
> +       axictl = tmc_readl(drvdata, TMC_AXICTL);
> +       axictl |= (0xF << 8);
> +       tmc_writel(drvdata, axictl, TMC_AXICTL);
> +       axictl &= ~(0x1 << 7);
> +       tmc_writel(drvdata, axictl, TMC_AXICTL);
> +       axictl = (axictl & ~0x3) | 0x2;
> +       tmc_writel(drvdata, axictl, TMC_AXICTL);

I don't understand these bits and shifts either.

> +       tmc_writel(drvdata, drvdata->paddr, TMC_DBALO);
> +       tmc_writel(drvdata, 0x0, TMC_DBAHI);
> +       tmc_writel(drvdata, 0x133, TMC_FFCR);

More magic...

> +       tmc_writel(drvdata, drvdata->trigger_cntr, TMC_TRG);
> +       __tmc_enable(drvdata);
> +
> +       TMC_LOCK(drvdata);
> +}
> +
> +static void __tmc_etf_enable(struct tmc_drvdata *drvdata)
> +{
> +       TMC_UNLOCK(drvdata);
> +
> +       tmc_writel(drvdata, TMC_MODE_HARDWARE_FIFO, TMC_MODE);
> +       tmc_writel(drvdata, 0x3, TMC_FFCR);
> +       tmc_writel(drvdata, 0x0, TMC_BUFWM);

More magic.

> +       __tmc_enable(drvdata);
> +
> +       TMC_LOCK(drvdata);
> +}
> +
> +static int tmc_enable(struct tmc_drvdata *drvdata, enum tmc_mode mode)
> +{
> +       int ret;
> +       unsigned long flags;
> +
> +       ret = clk_prepare_enable(drvdata->clk);
> +       if (ret)
> +               return ret;
> +
> +       spin_lock_irqsave(&drvdata->spinlock, flags);
> +       if (drvdata->reading) {
> +               spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +               clk_disable_unprepare(drvdata->clk);
> +               return -EBUSY;
> +       }
> +
> +       if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
> +               __tmc_etb_enable(drvdata);
> +       } else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
> +               __tmc_etr_enable(drvdata);
> +       } else {
> +               if (mode == TMC_MODE_CIRCULAR_BUFFER)
> +                       __tmc_etb_enable(drvdata);
> +               else
> +                       __tmc_etf_enable(drvdata);
> +       }
> +       drvdata->enable = true;
> +       spin_unlock_irqrestore(&drvdata->spinlock, flags);
> +
> +       dev_info(drvdata->dev, "TMC enabled\n");
> +       return 0;
> +}
> +
> +static int tmc_enable_sink(struct coresight_device *csdev)
> +{
> +       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       return tmc_enable(drvdata, TMC_MODE_CIRCULAR_BUFFER);
> +}
> +
> +static int tmc_enable_link(struct coresight_device *csdev, int inport,
> +                          int outport)
> +{
> +       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
> +
> +       return tmc_enable(drvdata, TMC_MODE_HARDWARE_FIFO);
> +}
> +
> +static void __tmc_etb_dump(struct tmc_drvdata *drvdata)

I'm no fan of prefixing functions with __underscores and cannot see why
this is done here even, just seems like force of habit. Please cut them.
Rename the function slightly to correspond to what it does if it collides
with another function.

> +{
> +       enum tmc_mem_intf_width memwidth;
> +       uint8_t memwords;

u8

> +       char *bufp;
> +       uint32_t read_data;

u32

(...)
> +       bufp = drvdata->buf;
> +       while (1) {
> +               for (i = 0; i < memwords; i++) {
> +                       read_data = tmc_readl(drvdata, TMC_RRD);
> +                       if (read_data == 0xFFFFFFFF)
> +                               return;
> +                       memcpy(bufp, &read_data, BYTES_PER_WORD);
> +                       bufp += BYTES_PER_WORD;

Use 4 rather than BYTES_PER_WORD please.

(...)
> +static void __tmc_etr_dump(struct tmc_drvdata *drvdata)

Cut __

> +{
> +       uint32_t rwp, rwphi;

u32

> +       rwp = tmc_readl(drvdata, TMC_RWP);
> +       rwphi = tmc_readl(drvdata, TMC_RWPHI);
> +
> +       if (BVAL(tmc_readl(drvdata, TMC_STS), 0))
> +               drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
> +       else
> +               drvdata->buf = drvdata->vaddr;
> +}
> +
> +static void __tmc_etr_disable(struct tmc_drvdata *drvdata)

Cut __

> +{
> +       TMC_UNLOCK(drvdata);
> +
> +       tmc_flush_and_stop(drvdata);
> +       __tmc_etr_dump(drvdata);
> +       __tmc_disable(drvdata);
> +
> +       TMC_LOCK(drvdata);
> +}
> +
> +static void __tmc_etf_disable(struct tmc_drvdata *drvdata)

Cut __

(...)
> +static int tmc_probe(struct platform_device *pdev)
> +{
> +       int ret = 0;
> +       uint32_t devid;

u32

> +       struct device *dev = &pdev->dev;
> +       struct coresight_platform_data *pdata = NULL;
> +       struct tmc_drvdata *drvdata;
> +       struct resource *res;
> +       struct coresight_desc *desc;
> +
> +       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 = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
> +       if (!drvdata)
> +               return -ENOMEM;
> +       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));

Use devm_ioremap_resource() instead.

> +       if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
> +               desc->type = CORESIGHT_DEV_TYPE_SINK;
> +               desc->subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> +               desc->ops = &tmc_etb_cs_ops;
> +               desc->pdata = pdev->dev.platform_data;
> +               desc->dev = &pdev->dev;
> +               desc->debugfs_ops = tmc_etb_attr_grps;
> +               desc->owner = THIS_MODULE;
> +               drvdata->csdev = coresight_register(desc);
> +               if (IS_ERR(drvdata->csdev)) {
> +                       ret = PTR_ERR(drvdata->csdev);
> +                       goto err0;
> +               }
> +       } else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
> +               desc->type = CORESIGHT_DEV_TYPE_SINK;
> +               desc->subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> +               desc->ops = &tmc_etr_cs_ops;
> +               desc->pdata = pdev->dev.platform_data;
> +               desc->dev = &pdev->dev;
> +               desc->debugfs_ops = tmc_etr_attr_grps;
> +               desc->owner = THIS_MODULE;
> +               drvdata->csdev = coresight_register(desc);
> +               if (IS_ERR(drvdata->csdev)) {
> +                       ret = PTR_ERR(drvdata->csdev);
> +                       goto err0;
> +               }
> +       } else {
> +               desc->type = CORESIGHT_DEV_TYPE_LINKSINK;
> +               desc->subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
> +               desc->subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
> +               desc->ops = &tmc_etf_cs_ops;
> +               desc->pdata = pdev->dev.platform_data;
> +               desc->dev = &pdev->dev;
> +               desc->debugfs_ops = tmc_etf_attr_grps;
> +               desc->owner = THIS_MODULE;
> +               drvdata->csdev = coresight_register(desc);
> +               if (IS_ERR(drvdata->csdev)) {
> +                       ret = PTR_ERR(drvdata->csdev);
> +                       goto err0;
> +               }
> +       }

Actually you set some stuff like desc->dev and pdata to the
same thing in all three clauses... Do it before the if/else-ladder
and cut the repititions.

> +       drvdata->miscdev.name = ((struct coresight_platform_data *)
> +                                (pdev->dev.platform_data))->name;
> +       drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
> +       drvdata->miscdev.fops = &tmc_fops;
> +       ret = misc_register(&drvdata->miscdev);

Miscdev really? Well, not that I know any better.

> +static struct platform_driver tmc_driver = {
> +       .probe          = tmc_probe,
> +       .remove         = tmc_remove,
> +       .driver         = {
> +               .name   = "coresight-tmc",
> +               .owner  = THIS_MODULE,
> +               .of_match_table = tmc_match,
> +       },
> +};
> +
> +static int __init tmc_init(void)
> +{
> +       return platform_driver_register(&tmc_driver);
> +}
> +module_init(tmc_init);
> +
> +static void __exit tmc_exit(void)
> +{
> +       platform_driver_unregister(&tmc_driver);
> +}
> +module_exit(tmc_exit);

Convert to use module_platform_driver()

I think these review comments apply to many of the patches,
so please take each comment and iterate over the code.

Yours,
Linus Walleij
Mathieu Poirier June 16, 2014, 11:12 p.m. UTC | #2
Thanks for the review - please see comments in-lined.

Mathieu

On 3 June 2014 03:09, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, May 30, 2014 at 3:43 PM,  <mathieu.poirier@linaro.org> wrote:
>
>> +#define tmc_writel(drvdata, val, off)  __raw_writel((val), drvdata->base + off)
>> +#define tmc_readl(drvdata, off)                __raw_readl(drvdata->base + off)
>
> Why not writel_relaxed()/readl_relaxed()?

Done.

>
> Using __raw* accessors seem a bit thick. (Applies to all such defines.)
>
>> +#define TMC_LOCK(drvdata)                                              \
>> +do {                                                                   \
>> +       /* settle everything first */                                   \
>> +       mb();                                                           \
>> +       tmc_writel(drvdata, 0x0, CORESIGHT_LAR);                        \
>> +} while (0)
>> +#define TMC_UNLOCK(drvdata)                                            \
>> +do {                                                                   \
>> +       tmc_writel(drvdata, CORESIGHT_UNLOCK, CORESIGHT_LAR);           \
>> +       /* make sure everyone sees this */                              \
>> +       mb();                                                           \
>> +} while (0)
>
> Convert these to static inlines. No need for them to be #defines
> at all really.

Done.

>
>> +#define BYTES_PER_WORD         4
>
> But please. Just using the number 4 everywhere is clear enough.
>
>> +struct tmc_drvdata {
>> +       void __iomem            *base;
>> +       struct device           *dev;
>> +       struct coresight_device *csdev;
>> +       struct miscdevice       miscdev;
>> +       struct clk              *clk;
>> +       spinlock_t              spinlock;
>> +       int                     read_count;
>
> Can this really be negative?

It is useful for debugging, as an example see "tmc_release()".  If the
count drops below '0' there is obviously a problem.  Do you see a cost
in keeping this as an 'int'?  What do you advise here?

>
>> +       bool                    reading;
>> +       char                    *buf;
>> +       dma_addr_t              paddr;
>> +       void __iomem            *vaddr;
>> +       uint32_t                size;
>
> Use u32
>
>> +       bool                    enable;
>> +       enum tmc_config_type    config_type;
>> +       uint32_t                trigger_cntr;
>
> Use u32
>
>> +};
>
> This struct overall could use some kerneldoc.

Would writing a comment for each field qualify?

>
>> +static void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
>> +{
>> +       int count;
>
> Why not call this variable "i" as per convention.
>
>> +       uint32_t ffcr;
>
> u32
>
>> +
>> +       ffcr = tmc_readl(drvdata, TMC_FFCR);
>> +       ffcr |= BIT(12);
>> +       tmc_writel(drvdata, ffcr, TMC_FFCR);
>> +       ffcr |= BIT(6);
>
> A bit unclear what bit 12 and 6 does. Either #define them or add comments.
>
>> +       tmc_writel(drvdata, ffcr, TMC_FFCR);
>> +       /* Ensure flush completes */
>> +       for (count = TIMEOUT_US; BVAL(tmc_readl(drvdata, TMC_FFCR), 6) != 0
>> +                               && count > 0; count--)
>> +               udelay(1);
>> +       WARN(count == 0, "timeout while flushing TMC, TMC_FFCR: %#x\n",
>> +            tmc_readl(drvdata, TMC_FFCR));
>> +
>> +       tmc_wait_for_ready(drvdata);
>> +}
>> +
>> +static void __tmc_enable(struct tmc_drvdata *drvdata)
>> +{
>> +       tmc_writel(drvdata, 0x1, TMC_CTL);
>> +}
>> +
>> +static void __tmc_disable(struct tmc_drvdata *drvdata)
>> +{
>> +       tmc_writel(drvdata, 0x0, TMC_CTL);
>> +}
>
> I actually understand what bit 0 does in this register, but could
> also be #defined.
>
>> +static void __tmc_etb_enable(struct tmc_drvdata *drvdata)
>> +{
>> +       /* Zero out the memory to help with debug */
>> +       memset(drvdata->buf, 0, drvdata->size);
>> +
>> +       TMC_UNLOCK(drvdata);
>> +
>> +       tmc_writel(drvdata, TMC_MODE_CIRCULAR_BUFFER, TMC_MODE);
>> +       tmc_writel(drvdata, 0x133, TMC_FFCR);
>
> 0x133? Que ce que c'est?
>
>> +       tmc_writel(drvdata, drvdata->trigger_cntr, TMC_TRG);
>> +       __tmc_enable(drvdata);
>> +
>> +       TMC_LOCK(drvdata);
>> +}
>> +
>> +static void __tmc_etr_enable(struct tmc_drvdata *drvdata)
>> +{
>> +       uint32_t axictl;
>
> u32
>
>> +       /* Zero out the memory to help with debug */
>> +       memset(drvdata->vaddr, 0, drvdata->size);
>> +
>> +       TMC_UNLOCK(drvdata);
>> +
>> +       tmc_writel(drvdata, drvdata->size / BYTES_PER_WORD, TMC_RSZ);
>> +       tmc_writel(drvdata, TMC_MODE_CIRCULAR_BUFFER, TMC_MODE);
>> +
>> +       axictl = tmc_readl(drvdata, TMC_AXICTL);
>> +       axictl |= (0xF << 8);
>> +       tmc_writel(drvdata, axictl, TMC_AXICTL);
>> +       axictl &= ~(0x1 << 7);
>> +       tmc_writel(drvdata, axictl, TMC_AXICTL);
>> +       axictl = (axictl & ~0x3) | 0x2;
>> +       tmc_writel(drvdata, axictl, TMC_AXICTL);
>
> I don't understand these bits and shifts either.
>
>> +       tmc_writel(drvdata, drvdata->paddr, TMC_DBALO);
>> +       tmc_writel(drvdata, 0x0, TMC_DBAHI);
>> +       tmc_writel(drvdata, 0x133, TMC_FFCR);
>
> More magic...
>
>> +       tmc_writel(drvdata, drvdata->trigger_cntr, TMC_TRG);
>> +       __tmc_enable(drvdata);
>> +
>> +       TMC_LOCK(drvdata);
>> +}
>> +
>> +static void __tmc_etf_enable(struct tmc_drvdata *drvdata)
>> +{
>> +       TMC_UNLOCK(drvdata);
>> +
>> +       tmc_writel(drvdata, TMC_MODE_HARDWARE_FIFO, TMC_MODE);
>> +       tmc_writel(drvdata, 0x3, TMC_FFCR);
>> +       tmc_writel(drvdata, 0x0, TMC_BUFWM);
>
> More magic.
>
>> +       __tmc_enable(drvdata);
>> +
>> +       TMC_LOCK(drvdata);
>> +}
>> +
>> +static int tmc_enable(struct tmc_drvdata *drvdata, enum tmc_mode mode)
>> +{
>> +       int ret;
>> +       unsigned long flags;
>> +
>> +       ret = clk_prepare_enable(drvdata->clk);
>> +       if (ret)
>> +               return ret;
>> +
>> +       spin_lock_irqsave(&drvdata->spinlock, flags);
>> +       if (drvdata->reading) {
>> +               spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +               clk_disable_unprepare(drvdata->clk);
>> +               return -EBUSY;
>> +       }
>> +
>> +       if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
>> +               __tmc_etb_enable(drvdata);
>> +       } else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
>> +               __tmc_etr_enable(drvdata);
>> +       } else {
>> +               if (mode == TMC_MODE_CIRCULAR_BUFFER)
>> +                       __tmc_etb_enable(drvdata);
>> +               else
>> +                       __tmc_etf_enable(drvdata);
>> +       }
>> +       drvdata->enable = true;
>> +       spin_unlock_irqrestore(&drvdata->spinlock, flags);
>> +
>> +       dev_info(drvdata->dev, "TMC enabled\n");
>> +       return 0;
>> +}
>> +
>> +static int tmc_enable_sink(struct coresight_device *csdev)
>> +{
>> +       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +       return tmc_enable(drvdata, TMC_MODE_CIRCULAR_BUFFER);
>> +}
>> +
>> +static int tmc_enable_link(struct coresight_device *csdev, int inport,
>> +                          int outport)
>> +{
>> +       struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
>> +
>> +       return tmc_enable(drvdata, TMC_MODE_HARDWARE_FIFO);
>> +}
>> +
>> +static void __tmc_etb_dump(struct tmc_drvdata *drvdata)
>
> I'm no fan of prefixing functions with __underscores and cannot see why
> this is done here even, just seems like force of habit. Please cut them.
> Rename the function slightly to correspond to what it does if it collides
> with another function.

Done.

>
>> +{
>> +       enum tmc_mem_intf_width memwidth;
>> +       uint8_t memwords;
>
> u8
>
>> +       char *bufp;
>> +       uint32_t read_data;
>
> u32
>
> (...)
>> +       bufp = drvdata->buf;
>> +       while (1) {
>> +               for (i = 0; i < memwords; i++) {
>> +                       read_data = tmc_readl(drvdata, TMC_RRD);
>> +                       if (read_data == 0xFFFFFFFF)
>> +                               return;
>> +                       memcpy(bufp, &read_data, BYTES_PER_WORD);
>> +                       bufp += BYTES_PER_WORD;
>
> Use 4 rather than BYTES_PER_WORD please.
>
> (...)
>> +static void __tmc_etr_dump(struct tmc_drvdata *drvdata)
>
> Cut __
>
>> +{
>> +       uint32_t rwp, rwphi;
>
> u32
>
>> +       rwp = tmc_readl(drvdata, TMC_RWP);
>> +       rwphi = tmc_readl(drvdata, TMC_RWPHI);
>> +
>> +       if (BVAL(tmc_readl(drvdata, TMC_STS), 0))
>> +               drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
>> +       else
>> +               drvdata->buf = drvdata->vaddr;
>> +}
>> +
>> +static void __tmc_etr_disable(struct tmc_drvdata *drvdata)
>
> Cut __
>
>> +{
>> +       TMC_UNLOCK(drvdata);
>> +
>> +       tmc_flush_and_stop(drvdata);
>> +       __tmc_etr_dump(drvdata);
>> +       __tmc_disable(drvdata);
>> +
>> +       TMC_LOCK(drvdata);
>> +}
>> +
>> +static void __tmc_etf_disable(struct tmc_drvdata *drvdata)
>
> Cut __
>
> (...)
>> +static int tmc_probe(struct platform_device *pdev)
>> +{
>> +       int ret = 0;
>> +       uint32_t devid;
>
> u32
>
>> +       struct device *dev = &pdev->dev;
>> +       struct coresight_platform_data *pdata = NULL;
>> +       struct tmc_drvdata *drvdata;
>> +       struct resource *res;
>> +       struct coresight_desc *desc;
>> +
>> +       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 = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
>> +       if (!drvdata)
>> +               return -ENOMEM;
>> +       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));
>
> Use devm_ioremap_resource() instead.

I'm working on it.

>
>> +       if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
>> +               desc->type = CORESIGHT_DEV_TYPE_SINK;
>> +               desc->subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>> +               desc->ops = &tmc_etb_cs_ops;
>> +               desc->pdata = pdev->dev.platform_data;
>> +               desc->dev = &pdev->dev;
>> +               desc->debugfs_ops = tmc_etb_attr_grps;
>> +               desc->owner = THIS_MODULE;
>> +               drvdata->csdev = coresight_register(desc);
>> +               if (IS_ERR(drvdata->csdev)) {
>> +                       ret = PTR_ERR(drvdata->csdev);
>> +                       goto err0;
>> +               }
>> +       } else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
>> +               desc->type = CORESIGHT_DEV_TYPE_SINK;
>> +               desc->subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>> +               desc->ops = &tmc_etr_cs_ops;
>> +               desc->pdata = pdev->dev.platform_data;
>> +               desc->dev = &pdev->dev;
>> +               desc->debugfs_ops = tmc_etr_attr_grps;
>> +               desc->owner = THIS_MODULE;
>> +               drvdata->csdev = coresight_register(desc);
>> +               if (IS_ERR(drvdata->csdev)) {
>> +                       ret = PTR_ERR(drvdata->csdev);
>> +                       goto err0;
>> +               }
>> +       } else {
>> +               desc->type = CORESIGHT_DEV_TYPE_LINKSINK;
>> +               desc->subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
>> +               desc->subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
>> +               desc->ops = &tmc_etf_cs_ops;
>> +               desc->pdata = pdev->dev.platform_data;
>> +               desc->dev = &pdev->dev;
>> +               desc->debugfs_ops = tmc_etf_attr_grps;
>> +               desc->owner = THIS_MODULE;
>> +               drvdata->csdev = coresight_register(desc);
>> +               if (IS_ERR(drvdata->csdev)) {
>> +                       ret = PTR_ERR(drvdata->csdev);
>> +                       goto err0;
>> +               }
>> +       }
>
> Actually you set some stuff like desc->dev and pdata to the
> same thing in all three clauses... Do it before the if/else-ladder
> and cut the repititions.

Sure thing.

>
>> +       drvdata->miscdev.name = ((struct coresight_platform_data *)
>> +                                (pdev->dev.platform_data))->name;
>> +       drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
>> +       drvdata->miscdev.fops = &tmc_fops;
>> +       ret = misc_register(&drvdata->miscdev);
>
> Miscdev really? Well, not that I know any better.
>
>> +static struct platform_driver tmc_driver = {
>> +       .probe          = tmc_probe,
>> +       .remove         = tmc_remove,
>> +       .driver         = {
>> +               .name   = "coresight-tmc",
>> +               .owner  = THIS_MODULE,
>> +               .of_match_table = tmc_match,
>> +       },
>> +};
>> +
>> +static int __init tmc_init(void)
>> +{
>> +       return platform_driver_register(&tmc_driver);
>> +}
>> +module_init(tmc_init);
>> +
>> +static void __exit tmc_exit(void)
>> +{
>> +       platform_driver_unregister(&tmc_driver);
>> +}
>> +module_exit(tmc_exit);
>
> Convert to use module_platform_driver()

Very well.

>
> I think these review comments apply to many of the patches,
> so please take each comment and iterate over the code.
>
> Yours,
> Linus Walleij
Linus Walleij July 7, 2014, 10:53 a.m. UTC | #3
On Tue, Jun 17, 2014 at 1:12 AM, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
> On 3 June 2014 03:09, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Fri, May 30, 2014 at 3:43 PM,  <mathieu.poirier@linaro.org> wrote:

>>> +       int                     read_count;
>>
>> Can this really be negative?
>
> It is useful for debugging, as an example see "tmc_release()".  If the
> count drops below '0' there is obviously a problem.  Do you see a cost
> in keeping this as an 'int'?  What do you advise here?

No big deal, keep it.

>> This struct overall could use some kerneldoc.
>
> Would writing a comment for each field qualify?

Like above the struct, see Documentation/kernel-doc-nano-HOWTO.txt

>>> +       drvdata->base = devm_ioremap(dev, res->start, resource_size(res));
>>
>> Use devm_ioremap_resource() instead.
>
> I'm working on it.

Bet it's fininshed now ;-)

Yours,
Linus Walleij
diff mbox

Patch

diff --git a/drivers/coresight/Kconfig b/drivers/coresight/Kconfig
index 88fd44a..224903b 100644
--- a/drivers/coresight/Kconfig
+++ b/drivers/coresight/Kconfig
@@ -7,3 +7,14 @@  menuconfig CORESIGHT
 	  the right series of components on user input via sysfs. It also
 	  provides status information to user space applications through
 	  the debugfs interface.
+
+if CORESIGHT
+
+config CORESIGHT_LINKS_AND_SINKS
+	bool "CoreSight Link and Sink drivers"
+	help
+	  This enables support for CoreSight link and sink drivers that are
+	  responsible for transporting and collecting the trace data
+	  respectively.
+
+endif
diff --git a/drivers/coresight/Makefile b/drivers/coresight/Makefile
index 218e3b5..16e26c5 100644
--- a/drivers/coresight/Makefile
+++ b/drivers/coresight/Makefile
@@ -3,3 +3,4 @@ 
 #
 obj-$(CONFIG_CORESIGHT) += coresight.o
 obj-$(CONFIG_OF) += of_coresight.o
+obj-$(CONFIG_CORESIGHT_LINKS_AND_SINKS) += coresight-tmc.o
diff --git a/drivers/coresight/coresight-tmc.c b/drivers/coresight/coresight-tmc.c
new file mode 100644
index 0000000..afcab7c
--- /dev/null
+++ b/drivers/coresight/coresight-tmc.c
@@ -0,0 +1,796 @@ 
+/* 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/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/miscdevice.h>
+#include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/dma-mapping.h>
+#include <linux/delay.h>
+#include <linux/spinlock.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/of_coresight.h>
+#include <linux/coresight.h>
+
+#include "coresight-priv.h"
+
+#define tmc_writel(drvdata, val, off)	__raw_writel((val), drvdata->base + off)
+#define tmc_readl(drvdata, off)		__raw_readl(drvdata->base + off)
+
+#define TMC_LOCK(drvdata)						\
+do {									\
+	/* settle everything first */					\
+	mb();								\
+	tmc_writel(drvdata, 0x0, CORESIGHT_LAR);			\
+} while (0)
+#define TMC_UNLOCK(drvdata)						\
+do {									\
+	tmc_writel(drvdata, CORESIGHT_UNLOCK, CORESIGHT_LAR);		\
+	/* make sure everyone sees this */				\
+	mb();								\
+} while (0)
+
+#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)
+
+#define BYTES_PER_WORD		4
+
+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 {
+	void __iomem		*base;
+	struct device		*dev;
+	struct coresight_device	*csdev;
+	struct miscdevice	miscdev;
+	struct clk		*clk;
+	spinlock_t		spinlock;
+	int			read_count;
+	bool			reading;
+	char			*buf;
+	dma_addr_t		paddr;
+	void __iomem		*vaddr;
+	uint32_t		size;
+	bool			enable;
+	enum tmc_config_type	config_type;
+	uint32_t		trigger_cntr;
+};
+
+static void tmc_wait_for_ready(struct tmc_drvdata *drvdata)
+{
+	int count;
+
+	/* Ensure formatter, unformatter and hardware fifo are empty */
+	for (count = TIMEOUT_US; BVAL(tmc_readl(drvdata, TMC_STS), 2) != 1
+				&& count > 0; count--)
+		udelay(1);
+	WARN(count == 0, "timeout while waiting for TMC ready, TMC_STS: %#x\n",
+	     tmc_readl(drvdata, TMC_STS));
+}
+
+static void tmc_flush_and_stop(struct tmc_drvdata *drvdata)
+{
+	int count;
+	uint32_t ffcr;
+
+	ffcr = tmc_readl(drvdata, TMC_FFCR);
+	ffcr |= BIT(12);
+	tmc_writel(drvdata, ffcr, TMC_FFCR);
+	ffcr |= BIT(6);
+	tmc_writel(drvdata, ffcr, TMC_FFCR);
+	/* Ensure flush completes */
+	for (count = TIMEOUT_US; BVAL(tmc_readl(drvdata, TMC_FFCR), 6) != 0
+				&& count > 0; count--)
+		udelay(1);
+	WARN(count == 0, "timeout while flushing TMC, TMC_FFCR: %#x\n",
+	     tmc_readl(drvdata, TMC_FFCR));
+
+	tmc_wait_for_ready(drvdata);
+}
+
+static void __tmc_enable(struct tmc_drvdata *drvdata)
+{
+	tmc_writel(drvdata, 0x1, TMC_CTL);
+}
+
+static void __tmc_disable(struct tmc_drvdata *drvdata)
+{
+	tmc_writel(drvdata, 0x0, TMC_CTL);
+}
+
+static void __tmc_etb_enable(struct tmc_drvdata *drvdata)
+{
+	/* Zero out the memory to help with debug */
+	memset(drvdata->buf, 0, drvdata->size);
+
+	TMC_UNLOCK(drvdata);
+
+	tmc_writel(drvdata, TMC_MODE_CIRCULAR_BUFFER, TMC_MODE);
+	tmc_writel(drvdata, 0x133, TMC_FFCR);
+	tmc_writel(drvdata, drvdata->trigger_cntr, TMC_TRG);
+	__tmc_enable(drvdata);
+
+	TMC_LOCK(drvdata);
+}
+
+static void __tmc_etr_enable(struct tmc_drvdata *drvdata)
+{
+	uint32_t axictl;
+
+	/* Zero out the memory to help with debug */
+	memset(drvdata->vaddr, 0, drvdata->size);
+
+	TMC_UNLOCK(drvdata);
+
+	tmc_writel(drvdata, drvdata->size / BYTES_PER_WORD, TMC_RSZ);
+	tmc_writel(drvdata, TMC_MODE_CIRCULAR_BUFFER, TMC_MODE);
+
+	axictl = tmc_readl(drvdata, TMC_AXICTL);
+	axictl |= (0xF << 8);
+	tmc_writel(drvdata, axictl, TMC_AXICTL);
+	axictl &= ~(0x1 << 7);
+	tmc_writel(drvdata, axictl, TMC_AXICTL);
+	axictl = (axictl & ~0x3) | 0x2;
+	tmc_writel(drvdata, axictl, TMC_AXICTL);
+
+	tmc_writel(drvdata, drvdata->paddr, TMC_DBALO);
+	tmc_writel(drvdata, 0x0, TMC_DBAHI);
+	tmc_writel(drvdata, 0x133, TMC_FFCR);
+	tmc_writel(drvdata, drvdata->trigger_cntr, TMC_TRG);
+	__tmc_enable(drvdata);
+
+	TMC_LOCK(drvdata);
+}
+
+static void __tmc_etf_enable(struct tmc_drvdata *drvdata)
+{
+	TMC_UNLOCK(drvdata);
+
+	tmc_writel(drvdata, TMC_MODE_HARDWARE_FIFO, TMC_MODE);
+	tmc_writel(drvdata, 0x3, TMC_FFCR);
+	tmc_writel(drvdata, 0x0, TMC_BUFWM);
+	__tmc_enable(drvdata);
+
+	TMC_LOCK(drvdata);
+}
+
+static int tmc_enable(struct tmc_drvdata *drvdata, enum tmc_mode mode)
+{
+	int ret;
+	unsigned long flags;
+
+	ret = clk_prepare_enable(drvdata->clk);
+	if (ret)
+		return ret;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading) {
+		spin_unlock_irqrestore(&drvdata->spinlock, flags);
+		clk_disable_unprepare(drvdata->clk);
+		return -EBUSY;
+	}
+
+	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
+		__tmc_etb_enable(drvdata);
+	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
+		__tmc_etr_enable(drvdata);
+	} else {
+		if (mode == TMC_MODE_CIRCULAR_BUFFER)
+			__tmc_etb_enable(drvdata);
+		else
+			__tmc_etf_enable(drvdata);
+	}
+	drvdata->enable = true;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC enabled\n");
+	return 0;
+}
+
+static int tmc_enable_sink(struct coresight_device *csdev)
+{
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	return tmc_enable(drvdata, TMC_MODE_CIRCULAR_BUFFER);
+}
+
+static int tmc_enable_link(struct coresight_device *csdev, int inport,
+			   int outport)
+{
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	return tmc_enable(drvdata, TMC_MODE_HARDWARE_FIFO);
+}
+
+static void __tmc_etb_dump(struct tmc_drvdata *drvdata)
+{
+	enum tmc_mem_intf_width memwidth;
+	uint8_t memwords;
+	char *bufp;
+	uint32_t read_data;
+	int i;
+
+	memwidth = BMVAL(tmc_readl(drvdata, CORESIGHT_DEVID), 8, 10);
+	if (memwidth == TMC_MEM_INTF_WIDTH_32BITS)
+		memwords = 1;
+	else if (memwidth == TMC_MEM_INTF_WIDTH_64BITS)
+		memwords = 2;
+	else if (memwidth == TMC_MEM_INTF_WIDTH_128BITS)
+		memwords = 4;
+	else
+		memwords = 8;
+
+	bufp = drvdata->buf;
+	while (1) {
+		for (i = 0; i < memwords; i++) {
+			read_data = tmc_readl(drvdata, TMC_RRD);
+			if (read_data == 0xFFFFFFFF)
+				return;
+			memcpy(bufp, &read_data, BYTES_PER_WORD);
+			bufp += BYTES_PER_WORD;
+		}
+	}
+}
+
+static void __tmc_etb_disable(struct tmc_drvdata *drvdata)
+{
+	TMC_UNLOCK(drvdata);
+
+	tmc_flush_and_stop(drvdata);
+	__tmc_etb_dump(drvdata);
+	__tmc_disable(drvdata);
+
+	TMC_LOCK(drvdata);
+}
+
+static void __tmc_etr_dump(struct tmc_drvdata *drvdata)
+{
+	uint32_t rwp, rwphi;
+
+	rwp = tmc_readl(drvdata, TMC_RWP);
+	rwphi = tmc_readl(drvdata, TMC_RWPHI);
+
+	if (BVAL(tmc_readl(drvdata, TMC_STS), 0))
+		drvdata->buf = drvdata->vaddr + rwp - drvdata->paddr;
+	else
+		drvdata->buf = drvdata->vaddr;
+}
+
+static void __tmc_etr_disable(struct tmc_drvdata *drvdata)
+{
+	TMC_UNLOCK(drvdata);
+
+	tmc_flush_and_stop(drvdata);
+	__tmc_etr_dump(drvdata);
+	__tmc_disable(drvdata);
+
+	TMC_LOCK(drvdata);
+}
+
+static void __tmc_etf_disable(struct tmc_drvdata *drvdata)
+{
+	TMC_UNLOCK(drvdata);
+
+	tmc_flush_and_stop(drvdata);
+	__tmc_disable(drvdata);
+
+	TMC_LOCK(drvdata);
+}
+
+static void tmc_disable(struct tmc_drvdata *drvdata, enum tmc_mode mode)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading)
+		goto out;
+
+	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
+		__tmc_etb_disable(drvdata);
+	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
+		__tmc_etr_disable(drvdata);
+	} else {
+		if (mode == TMC_MODE_CIRCULAR_BUFFER)
+			__tmc_etb_disable(drvdata);
+		else
+			__tmc_etf_disable(drvdata);
+	}
+out:
+	drvdata->enable = false;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	clk_disable_unprepare(drvdata->clk);
+
+	dev_info(drvdata->dev, "TMC disabled\n");
+}
+
+static void tmc_disable_sink(struct coresight_device *csdev)
+{
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	tmc_disable(drvdata, TMC_MODE_CIRCULAR_BUFFER);
+}
+
+static void tmc_disable_link(struct coresight_device *csdev, int inport,
+			     int outport)
+{
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+
+	tmc_disable(drvdata, TMC_MODE_HARDWARE_FIFO);
+}
+
+static void tmc_abort(struct coresight_device *csdev)
+{
+	struct tmc_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
+	unsigned long flags;
+	enum tmc_mode mode;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (drvdata->reading)
+		goto out0;
+
+	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
+		__tmc_etb_disable(drvdata);
+	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
+		__tmc_etr_disable(drvdata);
+	} else {
+		mode = tmc_readl(drvdata, TMC_MODE);
+		if (mode == TMC_MODE_CIRCULAR_BUFFER)
+			__tmc_etb_disable(drvdata);
+		else
+			goto out1;
+	}
+out0:
+	drvdata->enable = false;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC aborted\n");
+	return;
+out1:
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+}
+
+static const struct coresight_ops_sink tmc_sink_ops = {
+	.enable		= tmc_enable_sink,
+	.disable	= tmc_disable_sink,
+	.abort		= tmc_abort,
+};
+
+static const struct coresight_ops_link tmc_link_ops = {
+	.enable		= tmc_enable_link,
+	.disable	= tmc_disable_link,
+};
+
+static const struct coresight_ops tmc_etb_cs_ops = {
+	.sink_ops	= &tmc_sink_ops,
+};
+
+static const struct coresight_ops tmc_etr_cs_ops = {
+	.sink_ops	= &tmc_sink_ops,
+};
+
+static const struct coresight_ops tmc_etf_cs_ops = {
+	.sink_ops	= &tmc_sink_ops,
+	.link_ops	= &tmc_link_ops,
+};
+
+static int tmc_read_prepare(struct tmc_drvdata *drvdata)
+{
+	int ret;
+	unsigned long flags;
+	enum tmc_mode mode;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (!drvdata->enable)
+		goto out;
+
+	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
+		__tmc_etb_disable(drvdata);
+	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
+		__tmc_etr_disable(drvdata);
+	} else {
+		mode = tmc_readl(drvdata, TMC_MODE);
+		if (mode == TMC_MODE_CIRCULAR_BUFFER) {
+			__tmc_etb_disable(drvdata);
+		} else {
+			ret = -ENODEV;
+			goto err;
+		}
+	}
+out:
+	drvdata->reading = true;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC read start\n");
+	return 0;
+err:
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+	return ret;
+}
+
+static void tmc_read_unprepare(struct tmc_drvdata *drvdata)
+{
+	unsigned long flags;
+	enum tmc_mode mode;
+
+	spin_lock_irqsave(&drvdata->spinlock, flags);
+	if (!drvdata->enable)
+		goto out;
+
+	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
+		__tmc_etb_enable(drvdata);
+	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
+		__tmc_etr_enable(drvdata);
+	} else {
+		mode = tmc_readl(drvdata, TMC_MODE);
+		if (mode == TMC_MODE_CIRCULAR_BUFFER)
+			__tmc_etb_enable(drvdata);
+	}
+out:
+	drvdata->reading = false;
+	spin_unlock_irqrestore(&drvdata->spinlock, flags);
+
+	dev_info(drvdata->dev, "TMC read end\n");
+}
+
+static int tmc_open(struct inode *inode, struct file *file)
+{
+	struct tmc_drvdata *drvdata = container_of(file->private_data,
+						   struct tmc_drvdata, miscdev);
+	int ret = 0;
+
+	if (drvdata->read_count++)
+		goto out;
+
+	ret = tmc_read_prepare(drvdata);
+	if (ret)
+		return ret;
+out:
+	nonseekable_open(inode, file);
+
+	dev_dbg(drvdata->dev, "%s: successfully opened\n", __func__);
+	return 0;
+}
+
+static ssize_t tmc_read(struct file *file, char __user *data, size_t len,
+			loff_t *ppos)
+{
+	struct tmc_drvdata *drvdata = container_of(file->private_data,
+						   struct tmc_drvdata, miscdev);
+	char *bufp = drvdata->buf + *ppos;
+
+	if (*ppos + len > drvdata->size)
+		len = drvdata->size - *ppos;
+
+	if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
+		if (bufp == (char *)(drvdata->vaddr + drvdata->size))
+			bufp = drvdata->vaddr;
+		else if (bufp > (char *)(drvdata->vaddr + drvdata->size))
+			bufp -= drvdata->size;
+		if ((bufp + len) > (char *)(drvdata->vaddr + drvdata->size))
+			len = (char *)(drvdata->vaddr + drvdata->size) - bufp;
+	}
+
+	if (copy_to_user(data, bufp, len)) {
+		dev_dbg(drvdata->dev, "%s: copy_to_user failed\n", __func__);
+		return -EFAULT;
+	}
+
+	*ppos += len;
+
+	dev_dbg(drvdata->dev, "%s: %d bytes copied, %d bytes left\n",
+		__func__, len, (int) (drvdata->size - *ppos));
+	return len;
+}
+
+static int tmc_release(struct inode *inode, struct file *file)
+{
+	struct tmc_drvdata *drvdata = container_of(file->private_data,
+						   struct tmc_drvdata, miscdev);
+
+	if (--drvdata->read_count) {
+		if (drvdata->read_count < 0) {
+			WARN_ONCE(1, "mismatched close\n");
+			drvdata->read_count = 0;
+		}
+		goto out;
+	}
+
+	tmc_read_unprepare(drvdata);
+out:
+	dev_dbg(drvdata->dev, "%s: released\n", __func__);
+	return 0;
+}
+
+static const struct file_operations tmc_fops = {
+	.owner		= THIS_MODULE,
+	.open		= tmc_open,
+	.read		= tmc_read,
+	.release	= tmc_release,
+	.llseek		= no_llseek,
+};
+
+static ssize_t debugfs_trigger_cntr_read(struct file *file,
+					 char __user *user_buf,
+					 size_t count, loff_t *ppos)
+{
+	ssize_t ret;
+	struct tmc_drvdata *drvdata = file->private_data;
+	unsigned long val = drvdata->trigger_cntr;
+	char *buf = kmalloc(PAGE_SIZE, GFP_KERNEL);
+
+	ret =  scnprintf(buf, PAGE_SIZE, "%#lx\n", val);
+	return simple_read_from_buffer(user_buf, count, ppos, buf, ret);
+}
+
+static ssize_t debugfs_trigger_cntr_write(struct file *file,
+					  const char __user *user_buf,
+					  size_t count, loff_t *ppos)
+{
+	unsigned long val;
+	struct tmc_drvdata *drvdata = file->private_data;
+
+	if (sscanf(user_buf, "%lx", &val) != 1)
+		return -EINVAL;
+
+	drvdata->trigger_cntr = val;
+	return count;
+}
+
+static const struct file_operations debugfs_trigger_cntr_ops = {
+	.open = simple_open,
+	.read = debugfs_trigger_cntr_read,
+	.write =  debugfs_trigger_cntr_write,
+};
+
+static const struct coresight_ops_entry debugfs_trigger_cntr_entry = {
+	.name = "trigger_cntr",
+	.mode =  S_IRUGO | S_IWUSR,
+	.ops = &debugfs_trigger_cntr_ops,
+};
+
+static const struct coresight_ops_entry *tmc_etb_attr_grps[] = {
+	&debugfs_trigger_cntr_entry,
+	NULL,
+};
+
+static const struct coresight_ops_entry *tmc_etr_attr_grps[] = {
+	&debugfs_trigger_cntr_entry,
+	NULL,
+};
+
+static const struct coresight_ops_entry *tmc_etf_attr_grps[] = {
+	&debugfs_trigger_cntr_entry,
+	NULL,
+};
+
+static int tmc_probe(struct platform_device *pdev)
+{
+	int ret = 0;
+	uint32_t devid;
+	struct device *dev = &pdev->dev;
+	struct coresight_platform_data *pdata = NULL;
+	struct tmc_drvdata *drvdata;
+	struct resource *res;
+	struct coresight_desc *desc;
+
+	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 = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata)
+		return -ENOMEM;
+	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));
+	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;
+	}
+
+	devid = tmc_readl(drvdata, CORESIGHT_DEVID);
+	drvdata->config_type = BMVAL(devid, 6, 7);
+
+	if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
+		if (pdev->dev.of_node)
+			ret = of_property_read_u32(pdev->dev.of_node,
+				"arm,buffer-size", &drvdata->size);
+		if (ret)
+			drvdata->size = SZ_1M;
+	} else {
+		drvdata->size = tmc_readl(drvdata, TMC_RSZ) * BYTES_PER_WORD;
+	}
+
+	clk_disable_unprepare(drvdata->clk);
+
+	if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
+		drvdata->vaddr = dma_alloc_coherent(dev, drvdata->size,
+						&drvdata->paddr, GFP_KERNEL);
+		if (!drvdata->vaddr)
+			return -ENOMEM;
+		memset(drvdata->vaddr, 0, drvdata->size);
+		drvdata->buf = drvdata->vaddr;
+	} else {
+		drvdata->buf = devm_kzalloc(dev, drvdata->size, GFP_KERNEL);
+		if (!drvdata->buf)
+			return -ENOMEM;
+	}
+
+	desc = devm_kzalloc(dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc) {
+		ret = -ENOMEM;
+		goto err0;
+	}
+	if (drvdata->config_type == TMC_CONFIG_TYPE_ETB) {
+		desc->type = CORESIGHT_DEV_TYPE_SINK;
+		desc->subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
+		desc->ops = &tmc_etb_cs_ops;
+		desc->pdata = pdev->dev.platform_data;
+		desc->dev = &pdev->dev;
+		desc->debugfs_ops = tmc_etb_attr_grps;
+		desc->owner = THIS_MODULE;
+		drvdata->csdev = coresight_register(desc);
+		if (IS_ERR(drvdata->csdev)) {
+			ret = PTR_ERR(drvdata->csdev);
+			goto err0;
+		}
+	} else if (drvdata->config_type == TMC_CONFIG_TYPE_ETR) {
+		desc->type = CORESIGHT_DEV_TYPE_SINK;
+		desc->subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
+		desc->ops = &tmc_etr_cs_ops;
+		desc->pdata = pdev->dev.platform_data;
+		desc->dev = &pdev->dev;
+		desc->debugfs_ops = tmc_etr_attr_grps;
+		desc->owner = THIS_MODULE;
+		drvdata->csdev = coresight_register(desc);
+		if (IS_ERR(drvdata->csdev)) {
+			ret = PTR_ERR(drvdata->csdev);
+			goto err0;
+		}
+	} else {
+		desc->type = CORESIGHT_DEV_TYPE_LINKSINK;
+		desc->subtype.sink_subtype = CORESIGHT_DEV_SUBTYPE_SINK_BUFFER;
+		desc->subtype.link_subtype = CORESIGHT_DEV_SUBTYPE_LINK_FIFO;
+		desc->ops = &tmc_etf_cs_ops;
+		desc->pdata = pdev->dev.platform_data;
+		desc->dev = &pdev->dev;
+		desc->debugfs_ops = tmc_etf_attr_grps;
+		desc->owner = THIS_MODULE;
+		drvdata->csdev = coresight_register(desc);
+		if (IS_ERR(drvdata->csdev)) {
+			ret = PTR_ERR(drvdata->csdev);
+			goto err0;
+		}
+	}
+
+	drvdata->miscdev.name = ((struct coresight_platform_data *)
+				 (pdev->dev.platform_data))->name;
+	drvdata->miscdev.minor = MISC_DYNAMIC_MINOR;
+	drvdata->miscdev.fops = &tmc_fops;
+	ret = misc_register(&drvdata->miscdev);
+	if (ret)
+		goto err1;
+
+	dev_info(dev, "TMC initialized\n");
+	return 0;
+err1:
+	coresight_unregister(drvdata->csdev);
+err0:
+	dma_free_coherent(dev, drvdata->size, &drvdata->paddr, GFP_KERNEL);
+	return ret;
+}
+
+static int tmc_remove(struct platform_device *pdev)
+{
+	struct tmc_drvdata *drvdata = platform_get_drvdata(pdev);
+
+	misc_deregister(&drvdata->miscdev);
+	coresight_unregister(drvdata->csdev);
+	dma_free_coherent(drvdata->dev, drvdata->size, &drvdata->paddr,
+			  GFP_KERNEL);
+	return 0;
+}
+
+static struct of_device_id tmc_match[] = {
+	{.compatible = "arm,coresight-tmc"},
+	{}
+};
+
+static struct platform_driver tmc_driver = {
+	.probe          = tmc_probe,
+	.remove         = tmc_remove,
+	.driver         = {
+		.name   = "coresight-tmc",
+		.owner	= THIS_MODULE,
+		.of_match_table = tmc_match,
+	},
+};
+
+static int __init tmc_init(void)
+{
+	return platform_driver_register(&tmc_driver);
+}
+module_init(tmc_init);
+
+static void __exit tmc_exit(void)
+{
+	platform_driver_unregister(&tmc_driver);
+}
+module_exit(tmc_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("CoreSight Trace Memory Controller driver");