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?
> 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.
[...]
> +#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>)
[...]
> +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.
> +
> +/*
> + * 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.
> +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.
> +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?
> +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?
> +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.
> + 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.
> + 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.
> +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;
}
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
