Hi Suzuki,
On Thu, 6 Jun 2019 at 14:17, Suzuki K Poulose suzuki.poulose@arm.com wrote:
Hi Mike,
On 06/06/2019 13:59, Mike Leach wrote:
Hi Suzuki,
Thanks for reviewing this but v3 came out last night. I'll transfer over comments where relevant but some stuff has changed.
Apologies, I missed the v2 and realized it only very late. Thanks, I will look at the new version. Some comments inline.
rc = coresight_claim_device(drvdata->base);if (rc)goto cti_not_enabled;if (drvdata->ctidev.cpu >= 0) {
dev_info(drvdata->dev, "cti enable smp call for cpu %d\n",Please could we make it dev_dbg() instead ?
debug comment dropped in next set.
drvdata->ctidev.cpu);rc = smp_call_function_single(drvdata->ctidev.cpu,cti_enable_hw_smp_call,drvdata, 1);Is there any reason why these need to be bound to the CPU ?
This is following the practice of the ETM - where the device is CPU bound - run the code on the associated cpu.
OK
+#include <asm/local.h> +#include <linux/spinlock.h> +#include "coresight-priv.h"
+/*
- Device registers
- 0x000 - 0x144: CTI programming and status
- 0xEDC - 0xEF8: CTI integration test.
- 0xF00 - 0xFFC: Coresight management registers.
- */
+/* CTI programming registers */ +#define CTICONTROL 0x000 +#define CTIINTACK 0x010 +#define CTIAPPSET 0x014 +#define CTIAPPCLEAR 0x018 +#define CTIAPPPULSE 0x01C +#define CTIINEN(n) (0x020 + (4 * n)) +#define CTIOUTEN(n) (0x0A0 + (4 * n)) +#define CTITRIGINSTATUS 0x130 +#define CTITRIGOUTSTATUS 0x134 +#define CTICHINSTATUS 0x138 +#define CTICHOUTSTATUS 0x13C +#define CTIGATE 0x140 +#define ASICCTL 0x144 +/* Integration test registers */ +#define ITCHINACK 0xEDC /* WO CTI CSSoc 400 only*/ +#define ITTRIGINACK 0xEE0 /* WO CTI CSSoc 400 only*/ +#define ITCHOUT 0xEE4 /* WO RW-600 */ +#define ITTRIGOUT 0xEE8 /* WO RW-600 */ +#define ITCHOUTACK 0xEEC /* RO CTI CSSoc 400 only*/ +#define ITTRIGOUTACK 0xEF0 /* RO CTI CSSoc 400 only*/ +#define ITCHIN 0xEF4 /* RO */ +#define ITTRIGIN 0xEF8 /* RO */
Do we use the Integration Test registers at all ? If not we could remove them.
They are useful on occasion - CTI connections tend to be the poorest documented. At Mathieu's suggestion these are conditionally compiled in the next set.
OK.
+#define CTIINOUTEN_MAX 32
+/*
- CTI Trigger signal type definitions.
- Labels for certain trigger interconnections between the CTI
- and standard CS components.
- Must match values in ./include/dt-bindings/arm/coresight-cti.h
Why not have these common bits in a shared header file ?
Or simply include dt-bindings/arm/coresight-cti.h directly?
Thats fine too
+/*
- CTI can be bound to a CPU, or a system device.
- Reflect this in the return value and do not default to cpu 0
- */
+static int of_cti_get_cpu(const struct device_node *node) +{
int cpu;struct device_node *dn;dn = of_parse_phandle(node, "cpu", 0);/* CTI Affinity defaults to no cpu */if (!dn)return -1;cpu = of_cpu_node_to_id(dn);of_node_put(dn);/* No Affinity if no cpu nodes are found */return (cpu < 0) ? -1 : cpu;I think it is time to make the of_coresigth_get_cpu() return the real result than defaulting it to 0. The callers can decide what they want to do about it. And thus you may be able to reuse it here.
I considered that - I was concerned about breaking stuff if I missed a use case. Seemed simpler and more contained to implement something new.
Going forward, when we get to ACPI support, this would be another PITA. So lets fix it. I could send a separate patch if needed.
CTI work is the trigger for this so I'll include a preparatory patch in my next set that fixes it.
This looks out of place to me, but may be valid in the following patches.
It expands as we support more CTI cases in the device tree.
OK. It may be good to have the file named cti-platform.c instead of hardcoding of_cti etc, given the new changes upstream with ACPI series.
Then you'll like v3 - it applies on top of your ACPI set on coresight/next, with the above mentioned change.
Mike
Suzuki