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

[Linus Walleij]

On Fri, May 30, 2014 at 3:43 PM,  <mathieu.poirier@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/