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

[Mathieu Poirier]

Thanks for the review - please see comments in-lined.

Mathieu

On 3 June 2014 03:09, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> 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()?

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
--
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/