From: Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com> To: Mike Leach <mike.leach@linaro.org>, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-doc@vger.kernel.org Cc: mathieu.poirier@linaro.org Subject: Re: [PATCH v5 01/14] coresight: cti: Initial CoreSight CTI Driver Date: Mon, 25 Nov 2019 19:03:24 +0000 [thread overview] Message-ID: <fce270a6-03a7-f620-9ebf-5117c5dd7cc6@arm.com> (raw) In-Reply-To: <20191119231912.12768-2-mike.leach@linaro.org> 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@linaro.org> 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@arm.com>
WARNING: multiple messages have this Message-ID (diff)
From: Suzuki Kuruppassery Poulose <suzuki.poulose@arm.com> To: Mike Leach <mike.leach@linaro.org>, coresight@lists.linaro.org, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, linux-doc@vger.kernel.org Cc: mathieu.poirier@linaro.org Subject: Re: [PATCH v5 01/14] coresight: cti: Initial CoreSight CTI Driver Date: Mon, 25 Nov 2019 19:03:24 +0000 [thread overview] Message-ID: <fce270a6-03a7-f620-9ebf-5117c5dd7cc6@arm.com> (raw) In-Reply-To: <20191119231912.12768-2-mike.leach@linaro.org> 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@linaro.org> 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@arm.com> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-11-25 19:03 UTC|newest] Thread overview: 124+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-19 23:18 [PATCH v5 00/14] CoreSight CTI Driver Mike Leach 2019-11-19 23:18 ` Mike Leach 2019-11-19 23:18 ` [PATCH v5 01/14] coresight: cti: Initial " Mike Leach 2019-11-19 23:18 ` Mike Leach 2019-11-21 20:21 ` Mathieu Poirier 2019-11-21 20:21 ` Mathieu Poirier 2019-11-29 12:05 ` Mike Leach 2019-11-29 12:05 ` Mike Leach 2019-12-03 16:53 ` Mathieu Poirier 2019-12-03 16:53 ` Mathieu Poirier 2019-11-25 19:03 ` Suzuki Kuruppassery Poulose [this message] 2019-11-25 19:03 ` Suzuki Kuruppassery Poulose 2019-11-29 12:06 ` Mike Leach 2019-11-29 12:06 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 02/14] coresight: cti: Add sysfs coresight mgmt reg access Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-22 17:19 ` Mathieu Poirier 2019-11-22 17:19 ` Mathieu Poirier 2019-11-19 23:19 ` [PATCH v5 03/14] coresight: cti: Add sysfs access to program function regs Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 18:26 ` Suzuki Kuruppassery Poulose 2019-11-27 18:26 ` Suzuki Kuruppassery Poulose 2019-11-29 12:47 ` Mike Leach 2019-11-29 12:47 ` Mike Leach 2019-11-28 10:54 ` Suzuki Kuruppassery Poulose 2019-11-28 10:54 ` Suzuki Kuruppassery Poulose 2019-11-28 17:20 ` Mathieu Poirier 2019-11-28 17:20 ` Mathieu Poirier 2019-11-28 18:00 ` Suzuki Kuruppassery Poulose 2019-11-28 18:00 ` Suzuki Kuruppassery Poulose 2019-11-29 12:50 ` Mike Leach 2019-11-29 12:50 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 04/14] coresight: cti: Add sysfs trigger / channel programming API Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-22 18:40 ` Mathieu Poirier 2019-11-22 18:40 ` Mathieu Poirier 2019-11-27 18:40 ` Suzuki Kuruppassery Poulose 2019-11-27 18:40 ` Suzuki Kuruppassery Poulose 2019-11-29 13:01 ` Mike Leach 2019-11-29 13:01 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 05/14] dt-bindings: arm: Adds CoreSight CTI hardware definitions Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-20 19:06 ` Mathieu Poirier 2019-11-20 19:06 ` Mathieu Poirier 2019-11-20 22:39 ` Mike Leach 2019-11-20 22:39 ` Mike Leach 2019-11-22 23:33 ` Rob Herring 2019-11-22 23:33 ` Rob Herring 2019-11-29 13:50 ` Mike Leach 2019-11-29 13:50 ` Mike Leach 2019-11-29 14:12 ` Suzuki Kuruppassery Poulose 2019-11-29 14:12 ` Suzuki Kuruppassery Poulose 2019-11-28 18:38 ` Suzuki Kuruppassery Poulose 2019-11-28 18:38 ` Suzuki Kuruppassery Poulose 2019-11-29 13:57 ` Mike Leach 2019-11-29 13:57 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 06/14] coresight: cti: Add device tree support for v8 arch CTI Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-25 19:00 ` Mathieu Poirier 2019-11-25 19:00 ` Mathieu Poirier 2019-11-29 11:33 ` Suzuki Kuruppassery Poulose 2019-11-29 11:33 ` Suzuki Kuruppassery Poulose 2019-12-03 10:59 ` Mike Leach 2019-12-03 10:59 ` Mike Leach 2019-12-03 11:28 ` Suzuki Kuruppassery Poulose 2019-12-03 11:28 ` Suzuki Kuruppassery Poulose 2019-12-03 12:25 ` Mike Leach 2019-12-03 12:25 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 07/14] coresight: cti: Add device tree support for custom CTI Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-25 21:22 ` Mathieu Poirier 2019-11-25 21:22 ` Mathieu Poirier 2019-11-29 14:16 ` Suzuki Kuruppassery Poulose 2019-11-29 14:16 ` Suzuki Kuruppassery Poulose 2019-11-29 21:11 ` Mathieu Poirier 2019-11-29 21:11 ` Mathieu Poirier 2019-11-29 14:18 ` Suzuki Kuruppassery Poulose 2019-11-29 14:18 ` Suzuki Kuruppassery Poulose 2019-12-03 14:05 ` Mike Leach 2019-12-03 14:05 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 08/14] coresight: cti: Enable CTI associated with devices Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-25 22:45 ` Mathieu Poirier 2019-11-25 22:45 ` Mathieu Poirier 2019-12-05 16:33 ` Mike Leach 2019-12-05 16:33 ` Mike Leach 2019-11-29 18:28 ` Suzuki Kuruppassery Poulose 2019-11-29 18:28 ` Suzuki Kuruppassery Poulose 2019-11-29 21:25 ` Mathieu Poirier 2019-11-29 21:25 ` Mathieu Poirier 2019-12-05 16:33 ` Mike Leach 2019-12-05 16:33 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 09/14] coresight: cti: Add connection information to sysfs Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 18:09 ` Mathieu Poirier 2019-11-27 18:09 ` Mathieu Poirier 2019-12-06 16:24 ` Mike Leach 2019-12-06 16:24 ` Mike Leach 2019-12-02 9:47 ` Suzuki Kuruppassery Poulose 2019-12-02 9:47 ` Suzuki Kuruppassery Poulose 2019-12-06 16:24 ` Mike Leach 2019-12-06 16:24 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 10/14] dt-bindings: qcom: Add CTI options for qcom msm8916 Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 18:18 ` Mathieu Poirier 2019-11-27 18:18 ` Mathieu Poirier 2019-11-19 23:19 ` [PATCH v5 11/14] dt-bindings: arm: Juno platform - add CTI entries to device tree Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 18:25 ` Mathieu Poirier 2019-11-27 18:25 ` Mathieu Poirier 2019-11-19 23:19 ` [PATCH v5 12/14] dt-bindings: hisilicon: Add CTI bindings for hi-6220 Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 13/14] docs: coresight: Update documentation for CoreSight to cover CTI Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 19:00 ` Mathieu Poirier 2019-11-27 19:00 ` Mathieu Poirier 2019-12-02 10:43 ` Suzuki Kuruppassery Poulose 2019-12-02 10:43 ` Suzuki Kuruppassery Poulose 2019-12-06 17:39 ` Mike Leach 2019-12-06 17:39 ` Mike Leach 2019-11-19 23:19 ` [PATCH v5 14/14] docs: sysfs: coresight: Add sysfs ABI documentation for CTI Mike Leach 2019-11-19 23:19 ` Mike Leach 2019-11-27 19:08 ` Mathieu Poirier 2019-11-27 19:08 ` Mathieu Poirier
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=fce270a6-03a7-f620-9ebf-5117c5dd7cc6@arm.com \ --to=suzuki.poulose@arm.com \ --cc=coresight@lists.linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-doc@vger.kernel.org \ --cc=mathieu.poirier@linaro.org \ --cc=mike.leach@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.