On 19/11/2019 23:18, Mike Leach wrote:
This introduces a baseline CTI driver and associated configuration files.
Uses the platform agnostic naming standard for CoreSight devices, along
with a generic platform probing method that currently supports device
tree descriptions, but allows for the ACPI bindings to be added once these
have been defined for the CTI devices.
Driver will probe for the device on the AMBA bus, and load the CTI driver
on CoreSight ID match to CTI IDs in tables.
Initial sysfs support for enable / disable provided.
Default CTI interconnection data is generated based on hardware
register signal counts, with no additional connection information.
Signed-off-by: Mike Leach <mike.leach@xxxxxxxxxx>
Looks good to me. Some very minor nits, feel free to ignore if you are
not respinning the series.
+/*
+ * Look at the HW DEVID register for some of the HW settings.
+ * DEVID[15:8] - max number of in / out triggers.
+ */
+#define CTI_DEVID_MAXTRIGS(devid_val) (int)((devid_val & 0xFF00) >> 8)
BMVAL(devid_val, 15, 8)
+
+/* DEVID[19:16] - number of CTM channels */
+#define CTI_DEVID_CTMCHANNELS(devid_val) (int)((devid_val & 0xF0000) >> 16)
BMVAL(devid_val, 19, 16)
+
+static void cti_set_default_config(struct device *dev,
+ struct cti_drvdata *drvdata)
+{
+ struct cti_config *config = &drvdata->config;
+ u32 devid;
+
+ devid = readl_relaxed(drvdata->base + CORESIGHT_DEVID);
+ config->nr_trig_max = CTI_DEVID_MAXTRIGS(devid);
+
+ /*
+ * no current hardware should exceed this, but protect the driver
+ * in case of fault / out of spec hw
+ */
+ if (config->nr_trig_max > CTIINOUTEN_MAX) {
+ dev_warn_once(dev,
+ "Limiting HW MaxTrig value(%d) to driver max(%d)\n",
+ config->nr_trig_max, CTIINOUTEN_MAX);
+ config->nr_trig_max = CTIINOUTEN_MAX;
+ }
+
+ config->nr_ctm_channels = CTI_DEVID_CTMCHANNELS(devid);
+
+ /* Most regs default to 0 as zalloc'ed except...*/
+ config->trig_filter_enable = true;
+ config->ctigate = GENMASK(config->nr_ctm_channels - 1, 0);
+ atomic_set(&config->enable_req_count, 0);
+}
+
+/*
+ * Add a connection entry to the list of connections for this
+ * CTI device.
+ */
+int cti_add_connection_entry(struct device *dev, struct cti_drvdata *drvdata,
+ struct cti_trig_con *tc,
+ struct coresight_device *csdev,
+ const char *assoc_dev_name)
+{
+ struct cti_device *cti_dev = &drvdata->ctidev;
+
+ tc->con_dev = csdev;
+ /*
+ * Prefer actual associated CS device dev name to supplied value -
+ * which is likely to be node name / other conn name.
+ */
+ if (csdev)
+ tc->con_dev_name = devm_kstrdup(dev,
+ dev_name(&csdev->dev),
+ GFP_KERNEL);
+ else if (assoc_dev_name != NULL)
+ tc->con_dev_name = devm_kstrdup(dev,
+ assoc_dev_name, GFP_KERNEL);
+ list_add_tail(&tc->node, &cti_dev->trig_cons);
+ cti_dev->nr_trig_con++;
+
+ /* add connection usage bit info to overall info */
+ drvdata->config.trig_in_use |= tc->con_in->used_mask;
+ drvdata->config.trig_out_use |= tc->con_out->used_mask;
Do we need to make sure that they are exclusive ?
WARN_ON(drvdata->config.trig_in_use ^ ~(tc->con_in->used_mask));
WARN_ON(drvdata->config.trig_out_use ^ ~(tc->con_out->used_mask));
+/** cti ect operations **/
+int cti_enable(struct coresight_device *csdev)
+{
+ struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
+
+ /* enable hardware with refcount */
nit: left over comment from previous revision ?
+ return cti_enable_hw(drvdata);
+}
+
+int cti_disable(struct coresight_device *csdev)
+{
+ struct cti_drvdata *drvdata = csdev_to_cti_drvdata(csdev);
+
+ /* disable hardware with refcount */
same here ?
+ return cti_disable_hw(drvdata);
+}
+
+
+static int cti_probe(struct amba_device *adev, const struct amba_id *id)
+{
+ int ret = 0;
+ void __iomem *base;
+ struct device *dev = &adev->dev;
+ struct cti_drvdata *drvdata = NULL;
+ struct coresight_desc cti_desc;
+ struct coresight_platform_data *pdata = NULL;
+ struct resource *res = &adev->res;
+
+ /* driver data*/
+ drvdata = devm_kzalloc(dev, sizeof(*drvdata), GFP_KERNEL);
+ if (!drvdata) {
+ ret = -ENOMEM;
+ dev_info(dev, "%s, mem err\n", __func__);
dev_err() ? As they may have higher priority than "info" and will get
displayed in the rare chance of them getting hit.
+ goto err_out;
+ }
+
+ /* Validity for the resource is already checked by the AMBA core */
+ base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(base)) {
+ ret = PTR_ERR(base);
+ dev_info(dev, "%s, remap err\n", __func__);
same here, dev_err()
+ goto err_out;
+ }
+ drvdata->base = base;
+
+ dev_set_drvdata(dev, drvdata);
+
+ /* default CTI device info */
+ drvdata->ctidev.cpu = -1;
+ drvdata->ctidev.nr_trig_con = 0;
+ drvdata->ctidev.ctm_id = 0;
+ INIT_LIST_HEAD(&drvdata->ctidev.trig_cons);
+
+ spin_lock_init(&drvdata->spinlock);
+
+ /* initialise CTI driver config values */
+ cti_set_default_config(dev, drvdata);
+
+ /* Parse the .dts for connections and signals */
minor nit: I would not mention about ".dts" here. The function name is
implicit. You could actually remove that comment.
As mentioned above, the comments are minor nits. So you may add
with/without addressing them:
Reviewed-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
