| Message ID | 1401457391-12242-3-git-send-email-mathieu.poirier@linaro.org |
|---|---|
| State | New |
| Headers | show |
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
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
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 --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");