Re: [PATCH v2 04/27] coresight: Introduce support for Coresight Addrss Translation Unit

[Suzuki K Poulose <suzuki.poulose@arm.com>]

On 05/03/2018 09:25 PM, Mathieu Poirier wrote:
> On 3 May 2018 at 11:31, Mathieu Poirier <mathieu.poirier@linaro.org> wrote:
>> On Tue, May 01, 2018 at 10:10:34AM +0100, Suzuki K Poulose wrote:
>>> Add the initial support for Coresight Address Translation Unit, which
>>> augments the TMC in Coresight SoC-600 by providing an improved Scatter
>>> Gather mechanism. CATU is always connected to a single TMC-ETR and
>>> converts the AXI address with a translated address (from a given SG
>>> table with specific format). The CATU should be programmed in pass
>>> through mode and enabled if the ETR doesn't translation by CATU.
>>>
>>> This patch provides mechanism to enable/disable the CATU always in the
>>> pass through mode.
>>>
>>> We reuse the existing ports mechanism to link the TMC-ETR to the
>>> connected CATU.
>>>
>>> i.e, TMC-ETR:output_port0 -> CATU:input_port0
>>>
>>> Reference manual for  CATU component is avilable in version r2p0 of :
>>> "Arm Coresight System-on-Chip SoC-600 Technical Reference Manual",
>>> under Section 4.9.
>>
>> Please remove the part about the TRM as it is bound to change.

Ok, I will. Generally the TRM for a particular release (rXpY) doesn't
change, unless there is a change in either X or Y or both.

>>>
>>> +config CORESIGHT_CATU
>>> +     bool "Coresight Address Translation Unit (CATU) driver"
>>> +     depends on CORESIGHT_LINK_AND_SINK_TMC
>>> +     help
>>> +        Enable support for the Coresight Address Translation Unit (CATU).
>>> +        CATU supports a scatter gather table of 4K pages, with forward/backward
>>> +        lookup. CATU helps TMC ETR to use large physically non-contiguous trace
>>> +        buffer by translating the addersses used by ETR to the corresponding
>>> +        physical adderss by looking up the table.
>>
>> There is a couple of typos in the last sentence.
> 
> There's also a typo in the patch title.
> 

>>> diff --git a/drivers/hwtracing/coresight/coresight-catu.c b/drivers/hwtracing/coresight/coresight-catu.c
>>> new file mode 100644
>>> index 0000000..2cd69a6
>>> --- /dev/null
>>> +++ b/drivers/hwtracing/coresight/coresight-catu.c
>>> @@ -0,0 +1,195 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +
>>
>> Extra line
>>
>>> +/*
>>> + * Copyright (C) 2017 ARM Limited. All rights reserved.
>>
>> You sure you don't want to bump this to 2018?
>>


>>> + *
>>> + * Coresight Address Translation Unit support
>>> + *
>>> + * Author: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/device.h>
>>> +#include <linux/amba/bus.h>
>>> +#include <linux/io.h>
>>> +#include <linux/slab.h>
>>
>> List in alphabetical order is possible.
>>

>>> +static int catu_disable(struct coresight_device *csdev, void *__unused)
>>> +{
>>> +     int rc;
>>> +     struct catu_drvdata *catu_drvdata = csdev_to_catu_drvdata(csdev);
>>> +
>>> +     CS_UNLOCK(catu_drvdata->base);
>>> +     rc = catu_disable_hw(catu_drvdata);
>>> +     CS_LOCK(catu_drvdata->base);
>>> +
>>
>> I suppose you can remove the extra line as catu_enable() doesn't have one.
>>
>>> +     return rc;
>>> +}

>>> +     drvdata->base = base;
>>> +     catu_desc.pdata = pdata;
>>> +     catu_desc.dev = dev;
>>> +     catu_desc.groups = catu_groups;
>>> +     catu_desc.type = CORESIGHT_DEV_TYPE_HELPER;
>>> +     catu_desc.subtype.helper_subtype = CORESIGHT_DEV_SUBTYPE_HELPER_CATU;
>>> +     catu_desc.ops = &catu_ops;
>>> +     drvdata->csdev = coresight_register(&catu_desc);
>>> +     if (IS_ERR(drvdata->csdev))
>>> +             ret = PTR_ERR(drvdata->csdev);
>>> +     if (!ret)
>>> +             dev_info(drvdata->dev, "initialized\n");
>>
>> Please remove as it 1) doesn't convey HW related information and 2) the TMC
>> doesn't out put anything.
>>

>>> diff --git a/drivers/hwtracing/coresight/coresight-catu.h b/drivers/hwtracing/coresight/coresight-catu.h
>>> new file mode 100644
>>> index 0000000..cd58d6f
>>> --- /dev/null
>>> +++ b/drivers/hwtracing/coresight/coresight-catu.h
>>> @@ -0,0 +1,89 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +
>>
>> Extra line
>>
>>> +/*
>>> + * Copyright (C) 2017 ARM Limited. All rights reserved.
>>> + *
>>> + * Author: Suzuki K Poulose <suzuki.poulose@arm.com>
>>> + *
>>
>> Extra line. In coresight-catu.c there isn't one.
>>

>>> +#define CATU_STATUS_READY    8
>>> +#define CATU_STATUS_ADRERR   0
>>> +#define CATU_STATUS_AXIERR   4
>>> +
>>> +
>>
>> Extra line.
>>
>>> +#define CATU_IRQEN_ON                0x1
>>> +#define CATU_IRQEN_OFF               0x0
>>> +
>>> +
>>
>> Extra line.
>>

Will address all the above.

Cheers
Suzuki