On Thursday 10 November 2016 11:25 PM, Mark Rutland wrote:
On Thu, Nov 03, 2016 at 01:41:59AM -0400, Anurup M wrote:
From: Tan Xiaojun <tanxiaojun@xxxxxxxxxx>
The Hisilicon Djtag is an independent component which connects
with some other components in the SoC by Debug Bus. This driver
can be configured to access the registers of connecting components
(like L3 cache) during real time debugging.
Just to check, is this likely to be used in multi-socket hardware, and
if so, are instances always-on?
Yes, It could be used in multi-socket hardware also.
The sockets are always enabled after bootup. Sorry I didn't get the
Signed-off-by: Tan Xiaojun <tanxiaojun@xxxxxxxxxx>
Signed-off-by: John Garry <john.garry@xxxxxxxxxx>
Signed-off-by: Anurup M <anurup.m@xxxxxxxxxx>
---
drivers/soc/Kconfig | 1 +
drivers/soc/Makefile | 1 +
drivers/soc/hisilicon/Kconfig | 12 +
drivers/soc/hisilicon/Makefile | 1 +
drivers/soc/hisilicon/djtag.c | 639 ++++++++++++++++++++++++++++++++++++
include/linux/soc/hisilicon/djtag.h | 38 +++
6 files changed, 692 insertions(+)
create mode 100644 drivers/soc/hisilicon/Kconfig
create mode 100644 drivers/soc/hisilicon/Makefile
create mode 100644 drivers/soc/hisilicon/djtag.c
create mode 100644 include/linux/soc/hisilicon/djtag.h
Other than the PMU driver(s), what is going to use this?
If you don't have something in particular, please also place this under
drivers/perf/hisilicon, along with the PMU driver(s).
We can always move it later if necessary.
[...]
Arnd also suggested the same. Currently as there are no other users I
shall move it to
drivers/perf/hisilicon.
+#define SC_DJTAG_TIMEOUT 100000 /* 100ms */
This would be better as:
#define SC_DJTAG_TIMEOUT_US (100 * USEC_PER_MSEC)
(you'll need to include <linux/time64.h>)
[...]
OK.
+static void djtag_read32_relaxed(void __iomem *regs_base, u32 off, u32 *value)
+{
+ void __iomem *reg_addr = regs_base + off;
+
+ *value = readl_relaxed(reg_addr);
+}
+
+static void djtag_write32(void __iomem *regs_base, u32 off, u32 val)
+{
+ void __iomem *reg_addr = regs_base + off;
+
+ writel(val, reg_addr);
+}
I think these make the driver harder to read, especially given the read
function is void and takes an output pointer.
In either case you can call readl/writel directly with base + off for
the __iomem ptr. Please do that.
Ok.
+
+/*
+ * djtag_readwrite_v1/v2: djtag read/write interface
+ * @reg_base: djtag register base address
+ * @offset: register's offset
+ * @mod_sel: module selection
+ * @mod_mask: mask to select specific modules for write
+ * @is_w: write -> true, read -> false
+ * @wval: value to register for write
+ * @chain_id: which sub module for read
+ * @rval: value in register for read
+ *
+ * Return non-zero if error, else return 0.
+ */
+static int djtag_readwrite_v1(void __iomem *regs_base, u32 offset, u32 mod_sel,
+ u32 mod_mask, bool is_w, u32 wval, int chain_id, u32 *rval)
+{
+ u32 rd;
+ int timeout = SC_DJTAG_TIMEOUT;
+
+ if (!(mod_mask & CHAIN_UNIT_CFG_EN)) {
+ pr_warn("djtag: do nothing.\n");
+ return 0;
+ }
+
+ /* djtag mster enable & accelerate R,W */
+ djtag_write32(regs_base, SC_DJTAG_MSTR_EN,
+ DJTAG_NOR_CFG | DJTAG_MSTR_EN);
+
+ /* select module */
+ djtag_write32(regs_base, SC_DJTAG_DEBUG_MODULE_SEL, mod_sel);
+ djtag_write32(regs_base, SC_DJTAG_CHAIN_UNIT_CFG_EN,
+ mod_mask & CHAIN_UNIT_CFG_EN);
+
+ if (is_w) {
+ djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_W);
+ djtag_write32(regs_base, SC_DJTAG_MSTR_DATA, wval);
+ } else
+ djtag_write32(regs_base, SC_DJTAG_MSTR_WR, DJTAG_MSTR_R);
+
+ /* address offset */
+ djtag_write32(regs_base, SC_DJTAG_MSTR_ADDR, offset);
+
+ /* start to write to djtag register */
+ djtag_write32(regs_base, SC_DJTAG_MSTR_START_EN, DJTAG_MSTR_START_EN);
+
+ /* ensure the djtag operation is done */
+ do {
+ djtag_read32_relaxed(regs_base, SC_DJTAG_MSTR_START_EN, &rd);
+ if (!(rd & DJTAG_MSTR_EN))
+ break;
+
+ udelay(1);
+ } while (timeout--);
+
+ if (timeout < 0) {
+ pr_err("djtag: %s timeout!\n", is_w ? "write" : "read");
+ return -EBUSY;
+ }
+
+ if (!is_w)
+ djtag_read32_relaxed(regs_base,
+ SC_DJTAG_RD_DATA_BASE + chain_id * 0x4, rval);
+
+ return 0;
+}
Please factor out the common bits into helpers and have separate
read/write functions. It's incredibly difficult to follow the code with
read/write hidden behind a boolean parameter.
Ok. Shall change it.
+static const struct of_device_id djtag_of_match[] = {
+ /* for hip05(D02) cpu die */
+ { .compatible = "hisilicon,hip05-cpu-djtag-v1",
+ .data = (void *)djtag_readwrite_v1 },
+ /* for hip05(D02) io die */
+ { .compatible = "hisilicon,hip05-io-djtag-v1",
+ .data = (void *)djtag_readwrite_v1 },
+ /* for hip06(D03) cpu die */
+ { .compatible = "hisilicon,hip06-cpu-djtag-v1",
+ .data = (void *)djtag_readwrite_v1 },
+ /* for hip06(D03) io die */
+ { .compatible = "hisilicon,hip06-io-djtag-v2",
+ .data = (void *)djtag_readwrite_v2 },
+ /* for hip07(D05) cpu die */
+ { .compatible = "hisilicon,hip07-cpu-djtag-v2",
+ .data = (void *)djtag_readwrite_v2 },
+ /* for hip07(D05) io die */
+ { .compatible = "hisilicon,hip07-io-djtag-v2",
+ .data = (void *)djtag_readwrite_v2 },
+ {},
+};
Binding documentation for all of these should be added *before* this
patch, per Documentation/devicetree/bindings/submitting-patches.txt.
Please move any relevant binding documentation earlier in the series.
The binding documentation added in "[RESEND PATCH v1 02/11] dt-bindings:
hisi: Add Hisilicon HiP05/06/07 Sysctrl and Djtag dts bindings]".
+MODULE_DEVICE_TABLE(of, djtag_of_match);
+
+static ssize_t
+show_modalias(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct hisi_djtag_client *client = to_hisi_djtag_client(dev);
+
+ return sprintf(buf, "%s%s\n", MODULE_PREFIX, client->name);
+}
+static DEVICE_ATTR(modalias, 0444, show_modalias, NULL);
+
+static struct attribute *hisi_djtag_dev_attrs[] = {
+ NULL,
+ /* modalias helps coldplug: modprobe $(cat .../modalias) */
+ &dev_attr_modalias.attr,
+ NULL
+};
+ATTRIBUTE_GROUPS(hisi_djtag_dev);
Why do we need to expose this under sysfs?
This is to know the djtag clients registered with the bus.
+struct bus_type hisi_djtag_bus = {
+ .name = "hisi-djtag",
+ .match = hisi_djtag_device_match,
+ .probe = hisi_djtag_device_probe,
+ .remove = hisi_djtag_device_remove,
+};
I take it the core bus code handles reference-counting users of the bus?
The bus_register registers with kobject. Did It answer the question?
+struct hisi_djtag_client *hisi_djtag_new_device(struct hisi_djtag_host *host,
+ struct device_node *node)
+{
+ struct hisi_djtag_client *client;
+ int status;
+
+ client = kzalloc(sizeof(*client), GFP_KERNEL);
+ if (!client)
+ return NULL;
+
+ client->host = host;
+
+ client->dev.parent = &client->host->dev;
+ client->dev.bus = &hisi_djtag_bus;
+ client->dev.type = &hisi_djtag_client_type;
+ client->dev.of_node = node;
I suspect that we should do:
client->dev.of_node = of_node_get(node);
... so that it's guaranteed we retain a reference.
Ok.
+ snprintf(client->name, DJTAG_CLIENT_NAME_LEN, "%s%s",
+ DJTAG_PREFIX, node->name);
+ dev_set_name(&client->dev, "%s", client->name);
+
+ status = device_register(&client->dev);
+ if (status < 0) {
+ pr_err("error adding new device, status=%d\n", status);
+ kfree(client);
+ return NULL;
+ }
+
+ return client;
+}
+
+static struct hisi_djtag_client *hisi_djtag_of_register_device(
+ struct hisi_djtag_host *host,
+ struct device_node *node)
+{
+ struct hisi_djtag_client *client;
+
+ client = hisi_djtag_new_device(host, node);
+ if (client == NULL) {
+ dev_err(&host->dev, "error registering device %s\n",
+ node->full_name);
+ of_node_put(node);
I don't think this should be here, given what djtag_register_devices()
does.
Will move this to djtag_register_devices.
+ return ERR_PTR(-EINVAL);
+ }
+
+ return client;
+}
+
+static void djtag_register_devices(struct hisi_djtag_host *host)
+{
+ struct device_node *node;
+ struct hisi_djtag_client *client;
+
+ if (!host->of_node)
+ return;
+
+ for_each_available_child_of_node(host->of_node, node) {
+ if (of_node_test_and_set_flag(node, OF_POPULATED))
+ continue;
+ client = hisi_djtag_of_register_device(host, node);
+ list_add(&client->next, &host->client_list);
+ }
+}
Given hisi_djtag_of_register_device() can return ERR_PTR(-EINVAL), the
list_add is not safe.
Shall add the check and handle accordingly.
+static int djtag_host_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct hisi_djtag_host *host;
+ const struct of_device_id *of_id;
+ struct resource *res;
+ int rc;
+
+ of_id = of_match_device(djtag_of_match, dev);
+ if (!of_id)
+ return -EINVAL;
+
+ host = kzalloc(sizeof(*host), GFP_KERNEL);
+ if (!host)
+ return -ENOMEM;
+
+ host->of_node = dev->of_node;
host->of_node = of_node_get(dev->of_node);
+ host->djtag_readwrite = of_id->data;
+ spin_lock_init(&host->lock);
+
+ INIT_LIST_HEAD(&host->client_list);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "No reg resorces!\n");
+ kfree(host);
+ return -EINVAL;
+ }
+
+ if (!resource_size(res)) {
+ dev_err(&pdev->dev, "Zero reg entry!\n");
+ kfree(host);
+ return -EINVAL;
+ }
+
+ host->sysctl_reg_map = devm_ioremap_resource(dev, res);
+ if (IS_ERR(host->sysctl_reg_map)) {
+ dev_warn(dev, "Unable to map sysctl registers.\n");
+ kfree(host);
+ return -EINVAL;
+ }
Please have a common error path rather than duplicating this free.
e.g. at the end of the function have:
err_free:
kfree(host);
return err;
... and in cases like this, have:
if (whatever()) {
dev_warn(dev, "this failed xxx\n");
err = -EINVAL;
goto err_free;
}
Ok. thanks. will change it.
Thanks,
Anurup
Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
