On Thu, Oct 21, 2021 at 03:38:50PM +0800, Tao Zhang wrote:

Enable GPR and Basic Counts(BC) for TPDM. Add GPR interface and

But what is GPR?  What does it stand for?  Where is this documented?

There is no need for the double underscore at the beginning of the function
name, please remove.  The same goes for __tpdm_config_bc_msr() and
__tpdm_enable_bc().

+{
+	int i;
+
+	for (i = 0; i < TPDM_GPR_REGS_MAX; i++) {
+		if (!test_bit(i, drvdata->gpr->gpr_dirty))
+			continue;
+		tpdm_writel(drvdata, drvdata->gpr->gp_regs[i], TPDM_GPR_CR(i));
+	}
+}
+
+static void __tpdm_config_bc_msr(struct tpdm_drvdata *drvdata)
+{
+	int i;
+
+	if (!drvdata->msr_support)

Shouldn't msr_support be part of bc_dataset?  And why do we have a function for
this when an if() condition would work just as well, as it is done in
bc_trig_type below?

+		return;
+
+	for (i = 0; i < TPDM_BC_MAX_MSR; i++)
+		tpdm_writel(drvdata, drvdata->bc->msr[i], TPDM_BC_MSR(i));
+}
+
+static void __tpdm_enable_bc(struct tpdm_drvdata *drvdata)
+{
+	int i;
+	uint32_t val;
+
+	if (drvdata->bc->sat_mode)
+		tpdm_writel(drvdata, drvdata->bc->sat_mode,
+			    TPDM_BC_SATROLL);
+	else
+		tpdm_writel(drvdata, 0x0, TPDM_BC_SATROLL);
+
+	if (drvdata->bc->enable_counters) {
+		tpdm_writel(drvdata, 0xFFFFFFFF, TPDM_BC_CNTENCLR);
+		tpdm_writel(drvdata, drvdata->bc->enable_counters,
+			    TPDM_BC_CNTENSET);
+	}
+	if (drvdata->bc->clear_counters)
+		tpdm_writel(drvdata, drvdata->bc->clear_counters,
+			    TPDM_BC_CNTENCLR);
+
+	if (drvdata->bc->enable_irq) {
+		tpdm_writel(drvdata, 0xFFFFFFFF, TPDM_BC_INTENCLR);
+		tpdm_writel(drvdata, drvdata->bc->enable_irq,
+			    TPDM_BC_INTENSET);
+	}
+	if (drvdata->bc->clear_irq)
+		tpdm_writel(drvdata, drvdata->bc->clear_irq,
+			    TPDM_BC_INTENCLR);
+
+	if (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_FULL) {
+		for (i = 0; i < drvdata->bc_counters_avail; i++) {

Same here - shouldn't bc_trig_type and bc_counters_avail be part of bc_dataset?

+			tpdm_writel(drvdata, drvdata->bc->trig_val_lo[i],
+				    TPDM_BC_TRIG_LO(i));
+			tpdm_writel(drvdata, drvdata->bc->trig_val_hi[i],
+				    TPDM_BC_TRIG_HI(i));
+		}
+	} else if (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_PARTIAL) {
+		tpdm_writel(drvdata, drvdata->bc->trig_val_lo[0],
+			    TPDM_BC_TRIG_LO(0));
+		tpdm_writel(drvdata, drvdata->bc->trig_val_hi[0],
+			    TPDM_BC_TRIG_HI(0));
+	}
+
+	if (drvdata->bc->enable_ganging)
+		tpdm_writel(drvdata, drvdata->bc->enable_ganging, TPDM_BC_GANG);
+
+	for (i = 0; i < TPDM_BC_MAX_OVERFLOW; i++)
+		tpdm_writel(drvdata, drvdata->bc->overflow_val[i],
+			    TPDM_BC_OVERFLOW(i));
+
+	__tpdm_config_bc_msr(drvdata);
+
+	val = tpdm_readl(drvdata, TPDM_BC_CR);
+	if (drvdata->bc->retrieval_mode == TPDM_MODE_APB)
+		val = val | BIT(2);
+	else
+		val = val & ~BIT(2);
+	tpdm_writel(drvdata, val, TPDM_BC_CR);
+
+	val = tpdm_readl(drvdata, TPDM_BC_CR);
+	/* Set the enable bit */
+	val = val | BIT(0);
+	tpdm_writel(drvdata, val, TPDM_BC_CR);

As a whole, this function is very hard to read and understand due to the lack of
comments.

Shouldn't GPRs be disabled as well?  If not why is it the case?  A comment
should be explaining what is going on.

Everything related to sysfs should be in a patch on its own.

 
+static ssize_t available_datasets_show(struct device *dev,
+					    struct device_attribute *attr,
+					    char *buf)

Indentation problem here and for the rest of the patch.

+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+
+	if (test_bit(TPDM_DS_IMPLDEF, drvdata->datasets))
+		size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s",
+				  "IMPLDEF");
+
+	if (test_bit(TPDM_DS_DSB, drvdata->datasets))
+		size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "DSB");
+
+	if (test_bit(TPDM_DS_CMB, drvdata->datasets))
+		size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "CMB");
+
+	if (test_bit(TPDM_DS_TC, drvdata->datasets))
+		size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "TC");
+
+	if (test_bit(TPDM_DS_BC, drvdata->datasets))
+		size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "BC");
+
+	if (test_bit(TPDM_DS_GPR, drvdata->datasets))
+		size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "GPR");
+
+	if (test_bit(TPDM_DS_MCMB, drvdata->datasets))
+		size += scnprintf(buf + size, PAGE_SIZE - size, "%-8s", "MCMB");
+
+	size += scnprintf(buf + size, PAGE_SIZE - size, "\n");
+	return size;
+}
+static DEVICE_ATTR_RO(available_datasets);
+
+static ssize_t enable_datasets_show(struct device *dev,
+					 struct device_attribute *attr,
+					 char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size;
+
+	size = scnprintf(buf, PAGE_SIZE, "%*pb\n", TPDM_DATASETS,
+			 drvdata->enable_ds);
+
+	if (PAGE_SIZE - size < 2)
+		size = -EINVAL;

TPDM_DATASETS is set to 32 - is this realistic? 

+	else
+		size += scnprintf(buf + size, 2, "\n");

...and what is going on here?

+	return size;
+}
+
+static ssize_t enable_datasets_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf,
+					  size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long val;
+	int i;
+
+	if (kstrtoul(buf, 16, &val))
+		return -EINVAL;
+
+	mutex_lock(&drvdata->lock);
+	if (drvdata->enable) {
+		mutex_unlock(&drvdata->lock);
+		return -EPERM;
+	}
+
+	for (i = 0; i < TPDM_DATASETS; i++) {
+		if (test_bit(i, drvdata->datasets) && (val & BIT(i)))
+			__set_bit(i, drvdata->enable_ds);
+		else
+			__clear_bit(i, drvdata->enable_ds);
+	}
+	mutex_unlock(&drvdata->lock);
+	return size;
+}
+static DEVICE_ATTR_RW(enable_datasets);
+
+static ssize_t reset_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf,
+					  size_t size)
+{
+	int ret = 0;
+	unsigned long val;
+	struct mcmb_dataset *mcmb_temp = NULL;
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	ret = kstrtoul(buf, 10, &val);

The coresight subsystem normally uses the hexadecimal base.

+	if (ret)
+		return ret;

Shouldn't this be "if (!ret)" ? 

+
+	mutex_lock(&drvdata->lock);
+	/* Reset all datasets to ZERO */
+	if (drvdata->gpr != NULL)
+		memset(drvdata->gpr, 0, sizeof(struct gpr_dataset));
+
+	if (drvdata->bc != NULL)
+		memset(drvdata->bc, 0, sizeof(struct bc_dataset));
+
+	if (drvdata->tc != NULL)
+		memset(drvdata->tc, 0, sizeof(struct tc_dataset));
+
+	if (drvdata->dsb != NULL)
+		memset(drvdata->dsb, 0, sizeof(struct dsb_dataset));
+
+	if (drvdata->cmb != NULL) {
+		if (drvdata->cmb->mcmb != NULL) {
+			mcmb_temp = drvdata->cmb->mcmb;
+			memset(drvdata->cmb->mcmb, 0,
+				sizeof(struct mcmb_dataset));
+			}
+
+		memset(drvdata->cmb, 0, sizeof(struct cmb_dataset));
+		drvdata->cmb->mcmb = mcmb_temp;
+	}
+	/* Init the default data */
+	tpdm_init_default_data(drvdata);
+
+	mutex_unlock(&drvdata->lock);
+
+	/* Disable tpdm if enabled */
+	if (drvdata->enable)
+		coresight_disable(drvdata->csdev);

Why is this done out of the lock?

+
+	return size;
+}
+static DEVICE_ATTR_WO(reset);
+
+static ssize_t integration_test_store(struct device *dev,
+					  struct device_attribute *attr,
+					  const char *buf,
+					  size_t size)
+{
+	int i, ret = 0;
+	unsigned long val;
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	if (val != 1 && val != 2)
+		return -EINVAL;
+
+	if (!drvdata->enable)
+		return -EINVAL;
+
+	if (val == 1)
+		val = ATBCNTRL_VAL_64;
+	else
+		val = ATBCNTRL_VAL_32;
+	TPDM_UNLOCK(drvdata);
+	tpdm_writel(drvdata, 0x1, TPDM_ITCNTRL);
+
+	for (i = 1; i < 5; i++)
+		tpdm_writel(drvdata, val, TPDM_ITATBCNTRL);
+
+	tpdm_writel(drvdata, 0, TPDM_ITCNTRL);
+	TPDM_LOCK(drvdata);
+	return size;
+}
+static DEVICE_ATTR_WO(integration_test);

Integration test interface should be conditional to a compile time option.  Have
a look at what was done for CTIs.

+
+static ssize_t gp_regs_show(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	ssize_t size = 0;
+	int i = 0;
+
+	if (!test_bit(TPDM_DS_GPR, drvdata->datasets))
+		return -EPERM;

                return -EINVAL;

+
+	mutex_lock(&drvdata->lock);
+	for (i = 0; i < TPDM_GPR_REGS_MAX; i++) {
+		if (!test_bit(i, drvdata->gpr->gpr_dirty))
+			continue;
+		size += scnprintf(buf + size, PAGE_SIZE - size,
+				  "Index: 0x%x Value: 0x%x\n", i,
+				  drvdata->gpr->gp_regs[i]);

This should not be - the sysfs interface requires outputs of a single line.

+	}
+	mutex_unlock(&drvdata->lock);
+	return size;
+}
+
+static ssize_t gp_regs_store(struct device *dev,
+				  struct device_attribute *attr,
+				  const char *buf,
+				  size_t size)
+{
+	struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent);
+	unsigned long index, val;
+
+	if (sscanf(buf, "%lx %lx", &index, &val) != 2)
+		return -EINVAL;
+	if (!test_bit(TPDM_DS_GPR, drvdata->datasets) ||
+	    index >= TPDM_GPR_REGS_MAX)
+		return -EPERM;
+
+	mutex_lock(&drvdata->lock);
+	drvdata->gpr->gp_regs[index] = val;
+	__set_bit(index, drvdata->gpr->gpr_dirty);
+	mutex_unlock(&drvdata->lock);
+	return size;
+}
+static DEVICE_ATTR_RW(gp_regs);
+
+static struct attribute *tpdm_attrs[] = {
+	&dev_attr_available_datasets.attr,
+	&dev_attr_enable_datasets.attr,
+	&dev_attr_reset.attr,
+	&dev_attr_integration_test.attr,
+	&dev_attr_gp_regs.attr,
+	NULL,
+};

All new sysfs interface need to be documented.  See here:

Documentation/ABI/testing/sysfs-bus-coresight-devices-xyz

More comments to come...

Thanks,
Mathieu

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel