Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1605755 > unrolled thread

Re: [PATCH v6 06/11] drivers: perf: hisi: Add support for Hisilicon Djtag driver

Started byMark Rutland <mark.rutland@arm.com>
First post2017-03-21 17:00 +0100
Last post2017-03-27 08:40 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 06/11] drivers: perf: hisi: Add support for Hisilicon  Djtag driver Mark Rutland <mark.rutland@arm.com> - 2017-03-21 17:00 +0100
    Re: [PATCH v6 06/11] drivers: perf: hisi: Add support for Hisilicon  Djtag driver Anurup M <anurupvasu@gmail.com> - 2017-03-24 11:50 +0100
      Re: [PATCH v6 06/11] drivers: perf: hisi: Add support for Hisilicon  Djtag driver Mark Rutland <mark.rutland@arm.com> - 2017-03-24 12:40 +0100
        Re: [PATCH v6 06/11] drivers: perf: hisi: Add support for Hisilicon  Djtag driver Anurup M <anurupvasu@gmail.com> - 2017-03-27 08:40 +0200

#1605755 — Re: [PATCH v6 06/11] drivers: perf: hisi: Add support for Hisilicon Djtag driver

FromMark Rutland <mark.rutland@arm.com>
Date2017-03-21 17:00 +0100
SubjectRe: [PATCH v6 06/11] drivers: perf: hisi: Add support for Hisilicon Djtag driver
Message-ID<tnumu-5mU-17@gated-at.bofh.it>
On Fri, Mar 10, 2017 at 01:28:22AM -0500, Anurup M wrote:
> From: Tan Xiaojun <tanxiaojun@huawei.com>
> 
> 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.
> 
> Signed-off-by: Tan Xiaojun <tanxiaojun@huawei.com>
> Signed-off-by: John Garry <john.garry@huawei.com>
> Signed-off-by: Anurup M <anurup.m@huawei.com>
> ---
>  drivers/perf/Makefile           |   1 +
>  drivers/perf/hisilicon/Makefile |   1 +
>  drivers/perf/hisilicon/djtag.c  | 773 ++++++++++++++++++++++++++++++++++++++++
>  drivers/perf/hisilicon/djtag.h  |  42 +++
>  4 files changed, 817 insertions(+)
>  create mode 100644 drivers/perf/hisilicon/Makefile
>  create mode 100644 drivers/perf/hisilicon/djtag.c
>  create mode 100644 drivers/perf/hisilicon/djtag.h
> 
> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
> index 3a5e22f..d262fff 100644
> --- a/drivers/perf/Makefile
> +++ b/drivers/perf/Makefile
> @@ -1,4 +1,5 @@
>  obj-$(CONFIG_ARM_PMU) += arm_pmu.o
>  obj-$(CONFIG_QCOM_L2_PMU)	+= qcom_l2_pmu.o
> +obj-$(CONFIG_HISI_PMU) += hisilicon/

Please keep this ordered alphabetically. This should be between the
ARM_PMU and QCOM_L2_PMU cases.

>  obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
>  obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o

The cover letter said this was based upon v4.11-rc1, which does not
contain this last line.

What exactly is this series based on?

[...]

> +#define SC_DJTAG_TIMEOUT_US    (100 * USEC_PER_MSEC) /* 100ms */

How was this value chosen?

How likely is a timeout?

[...]

> +static DEFINE_IDR(djtag_hosts_idr);
> +static DEFINE_IDR(djtag_clients_idr);

Please include <linux/idr.h> for DEFINE_IDR().

[...]

> +struct hisi_djtag_host {
> +	spinlock_t lock;
> +	int id;
> +	u32 scl_id;
> +	struct device dev;

Please include <linux/device.h> for struct device.

> +	struct list_head client_list;
> +	void __iomem *sysctl_reg_map;
> +	struct device_node *of_node;
> +	const struct hisi_djtag_ops *djtag_ops;
> +};

[...]

> +static void djtag_prepare_v1(void __iomem *regs_base, u32 offset,
> +			     u32 mod_sel, u32 mod_mask)
> +{
> +	/* djtag master enable & accelerate R,W */
> +	writel(DJTAG_NOR_CFG | DJTAG_MSTR_EN, regs_base + SC_DJTAG_MSTR_EN);

I don't follow this comment. What exactly does this write do? What is
being accelerated?

Please include <linux/io.h> for the IO accessors.

[...]

> +static int djtag_do_operation_v1(void __iomem *regs_base)
> +{
> +	u32 rd;
> +	int timeout = SC_DJTAG_TIMEOUT_US;
> +
> +	/* start to write to djtag register */
> +	writel(DJTAG_MSTR_START_EN, regs_base + SC_DJTAG_MSTR_START_EN);
> +
> +	/* ensure the djtag operation is done */
> +	do {
> +		rd = readl(regs_base + SC_DJTAG_MSTR_START_EN);
> +		if (!(rd & DJTAG_MSTR_EN))
> +			break;
> +
> +		udelay(1);
> +	} while (timeout--);
> +
> +	if (timeout < 0)
> +		return -EBUSY;

Please include <linux/errno.h> for error values.

> +
> +	return 0;
> +}

Please use readl_poll_timeout(), e.g.

static int djtag_do_operation_v1(void __iomem *regs_base)
{
	int ret;
	u32 val;

	/* start to write to djtag register */
	writel(DJTAG_MSTR_START_EN, regs_base + SC_DJTAG_MSTR_START_EN);

	/* wait for the operation to complete */
	ret = readl_poll_timout(regs_base + SC_DJTAG_MSTR_START_EN,
				val, !(val & DJTAG_MSTR_EN),
				1, SC_DJTAG_TIMEOUT_US);
	
	if (ret)
		pr_warn("djtag operation timed out.\n");

	return ret;
}

Depending on how serious a timeout is, this might want to be some kind
of WARN variant.

Note that this will return -ETIMEDOUT when the condition is not met
before the timeout.

Please include <linux/iopoll.h> for this.

[...]

> +static int djtag_do_operation_v2(void __iomem *regs_base)
> +{
> +	u32 rd;
> +	int timeout = SC_DJTAG_TIMEOUT_US;
> +
> +	/* start to write to djtag register */
> +	writel(DJTAG_MSTR_START_EN_EX, regs_base + SC_DJTAG_MSTR_START_EN_EX);
> +
> +	/* ensure the djtag operation is done */
> +	do {
> +		rd = readl(regs_base + SC_DJTAG_MSTR_START_EN_EX);
> +
> +		if (!(rd & DJTAG_MSTR_START_EN_EX))
> +			break;
> +
> +		udelay(1);
> +	} while (timeout--);
> +
> +	if (timeout < 0)
> +		goto timeout;
> +
> +	timeout = SC_DJTAG_TIMEOUT_US;
> +	do {
> +		rd = readl(regs_base + SC_DJTAG_OP_ST_EX);
> +
> +		if (rd & DJTAG_OP_DONE_EX)
> +			break;
> +
> +		udelay(1);
> +	} while (timeout--);
> +
> +	if (timeout < 0)
> +		goto timeout;
> +
> +	return 0;
> +
> +timeout:
> +	return -EBUSY;
> +}

Likewise, please use readl_poll_timeout() for these.

[...]

> +static int djtag_read_v1(void __iomem *regs_base, u32 offset, u32 mod_sel,
> +			 u32 mod_mask, int chain_id, u32 *rval)
> +{
> +	int ret;
> +
> +	if (!(mod_mask & CHAIN_UNIT_CFG_EN)) {
> +		pr_warn("djtag: do nothing.\n");
> +		return 0;
> +	}

When does this happen, why should we warn, and why should we return?

> +
> +	djtag_prepare_v1(regs_base, offset, mod_sel, mod_mask);
> +
> +	writel(DJTAG_MSTR_R, regs_base + SC_DJTAG_MSTR_WR);
> +
> +	ret = djtag_do_operation_v1(regs_base);
> +	if (ret) {
> +		if (ret == EBUSY)
> +			pr_err("djtag: %s timeout!\n", "read");
> +		return ret;
> +	}

There's no reason to parameterise the message like this, and the only
non-zero return is a timeout, so we don't need to check the specific
error code.

> +
> +	*rval = readl(regs_base + SC_DJTAG_RD_DATA_BASE + chain_id * 0x4);
> +
> +	return 0;
> +}
> +
> +static int djtag_read_v2(void __iomem *regs_base, u32 offset, u32 mod_sel,
> +			 u32 mod_mask, int chain_id, u32 *rval)
> +{
> +	int ret;
> +
> +	if (!(mod_mask & CHAIN_UNIT_CFG_EN_EX)) {
> +		pr_warn("djtag: do nothing.\n");
> +		return 0;
> +	}
> +
> +	djtag_prepare_v2(regs_base, offset, mod_sel, mod_mask);
> +
> +	writel(DJTAG_MSTR_RD_EX
> +			| (mod_sel << DEBUG_MODULE_SEL_SHIFT_EX)
> +			| (mod_mask & CHAIN_UNIT_CFG_EN_EX),
> +			regs_base + SC_DJTAG_MSTR_CFG_EX);

This is rather painful to read, and violates kernel style. Each '|'
should be on the end of a line rather than starting the next one.

It would be nicer to generate the value in advance, and pass it in. e.g.

	u32 val = DJTAG_MSTR_RD_EX |
		  (mod_sel << DEBUG_MODULE_SEL_SHIFT_EX) |
		  (mod_mask & CHAIN_UNIT_CFG_EN_EX);
	writel(val, regs_base + SC_DJTAG_MSTR_CFG_EX);

> +
> +	ret = djtag_do_operation_v2(regs_base);
> +	if (ret) {
> +		if (ret == EBUSY)
> +			pr_err("djtag: %s timeout!\n", "read");
> +		return ret;
> +	}
> +
> +	*rval = readl(regs_base + SC_DJTAG_RD_DATA_BASE_EX +
> +					      chain_id * 0x4);

Weird alignment. Please align to the right of the '(', using spaces as
necessary, e.g.

	*rval = readl(regs_base + SC_DJTAG_RD_DATA_BASE_EX +
		      chain_id * 4);

> +
> +	return 0;
> +}
> +
> +/*
> + * djtag_write_v1/v2: djtag 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
> + * @wval:	value to register for write
> + * @chain_id:	which sub module for read
> + *
> + * Return non-zero if error, else return 0.
> + */
> +static int djtag_write_v1(void __iomem *regs_base, u32 offset, u32 mod_sel,
> +			  u32 mod_mask, u32 wval, int chain_id)
> +{
> +	int ret;
> +
> +	if (!(mod_mask & CHAIN_UNIT_CFG_EN)) {
> +		pr_warn("djtag: do nothing.\n");
> +		return 0;
> +	}
> +
> +	djtag_prepare_v1(regs_base, offset, mod_sel, mod_mask);
> +
> +	writel(DJTAG_MSTR_W, regs_base + SC_DJTAG_MSTR_WR);
> +	writel(wval, regs_base + SC_DJTAG_MSTR_DATA);
> +
> +	ret = djtag_do_operation_v1(regs_base);
> +	if (ret) {
> +		if (ret == EBUSY)
> +			pr_err("djtag: %s timeout!\n", "write");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

Same comments as with the read case.

> +static int djtag_write_v2(void __iomem *regs_base, u32 offset, u32 mod_sel,
> +			  u32 mod_mask, u32 wval, int chain_id)
> +{
> +	int ret;
> +
> +	if (!(mod_mask & CHAIN_UNIT_CFG_EN_EX)) {
> +		pr_warn("djtag: do nothing.\n");
> +		return 0;
> +	}
> +
> +	djtag_prepare_v2(regs_base, offset, mod_sel, mod_mask);
> +
> +	writel(DJTAG_MSTR_WR_EX
> +			| (mod_sel << DEBUG_MODULE_SEL_SHIFT_EX)
> +			| (mod_mask & CHAIN_UNIT_CFG_EN_EX),
> +			regs_base + SC_DJTAG_MSTR_CFG_EX);
> +	writel(wval, regs_base + SC_DJTAG_MSTR_DATA_EX);
> +
> +	ret = djtag_do_operation_v2(regs_base);
> +	if (ret) {
> +		if (ret == EBUSY)
> +			pr_err("djtag: %s timeout!\n", "write");
> +		return ret;
> +	}
> +
> +	return 0;
> +}

Likewise.

> +/**
> + * djtag_writel - write registers via djtag
> + * @client: djtag client handle
> + * @offset:	register's offset
> + * @mod_sel:	module selection
> + * @mod_mask:	mask to select specific modules
> + * @val:	value to write to register
> + *
> + * If error return errno, otherwise return 0.
> + */
> +int hisi_djtag_writel(struct hisi_djtag_client *client, u32 offset,
> +		      u32 mod_sel, u32 mod_mask, u32 val)

The function name doesn't match the comment block above.

> +{
> +	void __iomem *reg_map = client->host->sysctl_reg_map;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&client->host->lock, flags);
> +	ret = client->host->djtag_ops->djtag_write(reg_map, offset, mod_sel,
> +						   mod_mask, val, 0);
> +	if (ret)
> +		pr_err("djtag_writel: error! ret=%d\n", ret);
> +	spin_unlock_irqrestore(&client->host->lock, flags);
> +
> +	return ret;
> +}

Please use some temporary variables rather than a long chain of
dereferences.

AFAICT, the error here is pointless noise given the write op already
logs an error when it return a non-zero value.

So I think this can be:

int hisi_djtag_writel(struct hisi_djtag_client *client, u32 offset,
		      u32 mod_sel, u32 mod_mask, u32 val)
{
	struct hisi_djtag_host *host = client->host;
	struct hisi_djtag_ops *ops = host->djtag_ops;
	void __iomem *reg_map = host->sysctl_reg_map;
	unsigned long flags;
	int ret;

	spin_lock_irqsave(&client->host->lock, flags);
	ret = ops->djtag_write(reg_map, offset, mod_sel, mod_mask, val, 0);
	spin_unlock_irqrestore(&client->host->lock, flags);

	return ret;
}

Please consistently use the hisi_djtag_ prefix. It is confusing that
some functions have it while others do not.
	
> +/**
> + * djtag_readl - read registers via djtag
> + * @client: djtag client handle
> + * @offset:	register's offset
> + * @mod_sel:	module type selection
> + * @chain_id:	chain_id number, mostly is 0
> + * @val:	register's value
> + *
> + * If error return errno, otherwise return 0.
> + */
> +int hisi_djtag_readl(struct hisi_djtag_client *client, u32 offset,
> +		     u32 mod_sel, int chain_id, u32 *val)
> +{
> +	void __iomem *reg_map = client->host->sysctl_reg_map;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&client->host->lock, flags);
> +	ret = client->host->djtag_ops->djtag_read(reg_map, offset, mod_sel,
> +						  0xffff, chain_id, val);
> +	if (ret)
> +		pr_err("djtag_readl: error! ret=%d\n", ret);
> +	spin_unlock_irqrestore(&client->host->lock, flags);
> +
> +	return ret;
> +}

Same comments as for hisi_djtag_writel.

[...]

> +static struct attribute *hisi_djtag_dev_attrs[] = {
> +	NULL,
> +	/* modalias helps coldplug:  modprobe $(cat .../modalias) */
> +	&dev_attr_modalias.attr,
> +	NULL
> +};

Why is the first entry NULL?

[...]

> +static struct hisi_djtag_client *hisi_djtag_verify_client(struct device *dev)
> +{
> +	return (dev->type == &hisi_djtag_client_type)
> +			? to_hisi_djtag_client(dev)
> +			: NULL;
> +}

s/verify/get/

Why would this be called for a device which is not a djtag client?

[...]

> +static int hisi_djtag_device_match(struct device *dev,
> +				   struct device_driver *drv)
> +{
> +	struct hisi_djtag_client *client = hisi_djtag_verify_client(dev);
> +
> +	if (!client)
> +		return false;

Does this case ever happen?

[...]

> +	snprintf(client->name, DJTAG_CLIENT_NAME_LEN, "%s%s_%d",
> +		 DJTAG_PREFIX, device_name, client->id);
> +	dev_set_name(&client->dev, "%s", client->name);

This implies that hisi_djtag_client::name is redundant.

Please get rid of it, and use the name of hisi_djtag_client::dev where
necessary.

[...]

> +static void djtag_register_devices(struct hisi_djtag_host *host)
> +{
> +	struct device_node *node;
> +
> +	if (host->of_node) {

This is always true since the driver only probes via DT.

Please get rid of this check until it becomes necessary.

[...]

> +static int djtag_host_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct hisi_djtag_host *host;
> +	struct resource *res;
> +	int rc;

s/rc/ret/ for consistency with other code in this directory.

That applies to all instances in this patch.

I'm aware the xgene PMU doesn't stick to that style, and that was a
mistake.

> +	host = kzalloc(sizeof(*host), GFP_KERNEL);
> +	if (!host)
> +		return -ENOMEM;
> +
> +	if (dev->of_node) {

Likewise this should always be the case, since the driver only probes
via DT.

Please get rid of this check until it becomes necessary.

[...]

> +	if (!resource_size(res)) {
> +		dev_err(dev, "Zero reg entry!\n");
> +		rc = -EINVAL;
> +		goto fail;
> +	}

... but any non-zero size is fine?

If you want to check the size, please check it meets your minimum
requirement.

Thanks,
Mark.

[toc] | [next] | [standalone]

#1608308

FromAnurup M <anurupvasu@gmail.com>
Date2017-03-24 11:50 +0100
Message-ID<touX8-gm-15@gated-at.bofh.it>
In reply to#1605755
Thanks for the review.

On Tuesday 21 March 2017 09:21 PM, Mark Rutland wrote:
> On Fri, Mar 10, 2017 at 01:28:22AM -0500, Anurup M wrote:
>> From: Tan Xiaojun <tanxiaojun@huawei.com>
>>
>> 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.
>>
>> Signed-off-by: Tan Xiaojun <tanxiaojun@huawei.com>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> Signed-off-by: Anurup M <anurup.m@huawei.com>
>> ---
>>   drivers/perf/Makefile           |   1 +
>>   drivers/perf/hisilicon/Makefile |   1 +
>>   drivers/perf/hisilicon/djtag.c  | 773 ++++++++++++++++++++++++++++++++++++++++
>>   drivers/perf/hisilicon/djtag.h  |  42 +++
>>   4 files changed, 817 insertions(+)
>>   create mode 100644 drivers/perf/hisilicon/Makefile
>>   create mode 100644 drivers/perf/hisilicon/djtag.c
>>   create mode 100644 drivers/perf/hisilicon/djtag.h
>>
>> diff --git a/drivers/perf/Makefile b/drivers/perf/Makefile
>> index 3a5e22f..d262fff 100644
>> --- a/drivers/perf/Makefile
>> +++ b/drivers/perf/Makefile
>> @@ -1,4 +1,5 @@
>>   obj-$(CONFIG_ARM_PMU) += arm_pmu.o
>>   obj-$(CONFIG_QCOM_L2_PMU)	+= qcom_l2_pmu.o
>> +obj-$(CONFIG_HISI_PMU) += hisilicon/
> Please keep this ordered alphabetically. This should be between the
> ARM_PMU and QCOM_L2_PMU cases.

Ok.

>>   obj-$(CONFIG_XGENE_PMU) += xgene_pmu.o
>>   obj-$(CONFIG_ARM_PMU_ACPI) += arm_pmu_acpi.o
> The cover letter said this was based upon v4.11-rc1, which does not
> contain this last line.
>
> What exactly is this series based on?

Apologies. Mistake due to internal repo added ARMv8 ACPI patches. Shall 
take care of this and use 4.11.-rc1 linux branch for patches.

> [...]
>
>> +#define SC_DJTAG_TIMEOUT_US    (100 * USEC_PER_MSEC) /* 100ms */
> How was this value chosen?
>
> How likely is a timeout?

As explained in PATCH 7,

The djtag -EBUSY in hardware is a very rare scenario, and by design of 
hardware, does not occur unless there is a Chip hung situation.
The maximum timeout possible in djtag is 30us, and hardware logic shall 
reset it, if djtag is unavailable for more than 30us.
The timeout used in driver is 100ms to ensure that it does not fail in 
any case.

> [...]
>
>> +static DEFINE_IDR(djtag_hosts_idr);
>> +static DEFINE_IDR(djtag_clients_idr);
> Please include <linux/idr.h> for DEFINE_IDR().

Ok.

> [...]
>
>> +struct hisi_djtag_host {
>> +	spinlock_t lock;
>> +	int id;
>> +	u32 scl_id;
>> +	struct device dev;
> Please include <linux/device.h> for struct device.

Ok.

>> +	struct list_head client_list;
>> +	void __iomem *sysctl_reg_map;
>> +	struct device_node *of_node;
>> +	const struct hisi_djtag_ops *djtag_ops;
>> +};
> [...]
>
>> +static void djtag_prepare_v1(void __iomem *regs_base, u32 offset,
>> +			     u32 mod_sel, u32 mod_mask)
>> +{
>> +	/* djtag master enable & accelerate R,W */
>> +	writel(DJTAG_NOR_CFG | DJTAG_MSTR_EN, regs_base + SC_DJTAG_MSTR_EN);
> I don't follow this comment. What exactly does this write do? What is
> being accelerated?

The bit DJTAG_NOR_CFG will enable normal access mode for djtag. There is 
also low speed access mode.
The DJTAG_MSTR_EN will enable the djtag access.
I shall modify comment as

+	/* djtag master enable & support normal mode for R,W */



> Please include <linux/io.h> for the IO accessors.

Ok.

> [...]
>
>> +static int djtag_do_operation_v1(void __iomem *regs_base)
>> +{
>> +	u32 rd;
>> +	int timeout = SC_DJTAG_TIMEOUT_US;
>> +
>> +	/* start to write to djtag register */
>> +	writel(DJTAG_MSTR_START_EN, regs_base + SC_DJTAG_MSTR_START_EN);
>> +
>> +	/* ensure the djtag operation is done */
>> +	do {
>> +		rd = readl(regs_base + SC_DJTAG_MSTR_START_EN);
>> +		if (!(rd & DJTAG_MSTR_EN))
>> +			break;
>> +
>> +		udelay(1);
>> +	} while (timeout--);
>> +
>> +	if (timeout < 0)
>> +		return -EBUSY;
> Please include <linux/errno.h> for error values.

Ok.

>> +
>> +	return 0;
>> +}
> Please use readl_poll_timeout(), e.g.
>
> static int djtag_do_operation_v1(void __iomem *regs_base)
> {
> 	int ret;
> 	u32 val;
>
> 	/* start to write to djtag register */
> 	writel(DJTAG_MSTR_START_EN, regs_base + SC_DJTAG_MSTR_START_EN);
>
> 	/* wait for the operation to complete */
> 	ret = readl_poll_timout(regs_base + SC_DJTAG_MSTR_START_EN,
> 				val, !(val & DJTAG_MSTR_EN),
> 				1, SC_DJTAG_TIMEOUT_US);
> 	
> 	if (ret)
> 		pr_warn("djtag operation timed out.\n");
>
> 	return ret;
> }
>
> Depending on how serious a timeout is, this might want to be some kind
> of WARN variant.
>
> Note that this will return -ETIMEDOUT when the condition is not met
> before the timeout.

Thanks. I shall try this and update.

> Please include <linux/iopoll.h> for this.
>
> [...]

Ok.

>> +static int djtag_do_operation_v2(void __iomem *regs_base)
>> +{
>> +	u32 rd;
>> +	int timeout = SC_DJTAG_TIMEOUT_US;
>> +
>> +	/* start to write to djtag register */
>> +	writel(DJTAG_MSTR_START_EN_EX, regs_base + SC_DJTAG_MSTR_START_EN_EX);
>> +
>> +	/* ensure the djtag operation is done */
>> +	do {
>> +		rd = readl(regs_base + SC_DJTAG_MSTR_START_EN_EX);
>> +
>> +		if (!(rd & DJTAG_MSTR_START_EN_EX))
>> +			break;
>> +
>> +		udelay(1);
>> +	} while (timeout--);
>> +
>> +	if (timeout < 0)
>> +		goto timeout;
>> +
>> +	timeout = SC_DJTAG_TIMEOUT_US;
>> +	do {
>> +		rd = readl(regs_base + SC_DJTAG_OP_ST_EX);
>> +
>> +		if (rd & DJTAG_OP_DONE_EX)
>> +			break;
>> +
>> +		udelay(1);
>> +	} while (timeout--);
>> +
>> +	if (timeout < 0)
>> +		goto timeout;
>> +
>> +	return 0;
>> +
>> +timeout:
>> +	return -EBUSY;
>> +}
> Likewise, please use readl_poll_timeout() for these.
>
> [...]
>
>> +static int djtag_read_v1(void __iomem *regs_base, u32 offset, u32 mod_sel,
>> +			 u32 mod_mask, int chain_id, u32 *rval)
>> +{
>> +	int ret;
>> +
>> +	if (!(mod_mask & CHAIN_UNIT_CFG_EN)) {
>> +		pr_warn("djtag: do nothing.\n");
>> +		return 0;
>> +	}
> When does this happen, why should we warn, and why should we return?

Agreed, the value of mod_mask used is always >=1 so this check shall be 
removed.

>> +
>> +	djtag_prepare_v1(regs_base, offset, mod_sel, mod_mask);
>> +
>> +	writel(DJTAG_MSTR_R, regs_base + SC_DJTAG_MSTR_WR);
>> +
>> +	ret = djtag_do_operation_v1(regs_base);
>> +	if (ret) {
>> +		if (ret == EBUSY)
>> +			pr_err("djtag: %s timeout!\n", "read");
>> +		return ret;
>> +	}
> There's no reason to parameterise the message like this, and the only
> non-zero return is a timeout, so we don't need to check the specific
> error code.

Agreed. Shall correct it.

>> +
>> +	*rval = readl(regs_base + SC_DJTAG_RD_DATA_BASE + chain_id * 0x4);
>> +
>> +	return 0;
>> +}
>> +
>> +static int djtag_read_v2(void __iomem *regs_base, u32 offset, u32 mod_sel,
>> +			 u32 mod_mask, int chain_id, u32 *rval)
>> +{
>> +	int ret;
>> +
>> +	if (!(mod_mask & CHAIN_UNIT_CFG_EN_EX)) {
>> +		pr_warn("djtag: do nothing.\n");
>> +		return 0;
>> +	}
>> +
>> +	djtag_prepare_v2(regs_base, offset, mod_sel, mod_mask);
>> +
>> +	writel(DJTAG_MSTR_RD_EX
>> +			| (mod_sel << DEBUG_MODULE_SEL_SHIFT_EX)
>> +			| (mod_mask & CHAIN_UNIT_CFG_EN_EX),
>> +			regs_base + SC_DJTAG_MSTR_CFG_EX);
> This is rather painful to read, and violates kernel style. Each '|'
> should be on the end of a line rather than starting the next one.
>
> It would be nicer to generate the value in advance, and pass it in. e.g.
>
> 	u32 val = DJTAG_MSTR_RD_EX |
> 		  (mod_sel << DEBUG_MODULE_SEL_SHIFT_EX) |
> 		  (mod_mask & CHAIN_UNIT_CFG_EN_EX);
> 	writel(val, regs_base + SC_DJTAG_MSTR_CFG_EX);

Thanks, shall correct it.

>> +
>> +	ret = djtag_do_operation_v2(regs_base);
>> +	if (ret) {
>> +		if (ret == EBUSY)
>> +			pr_err("djtag: %s timeout!\n", "read");
>> +		return ret;
>> +	}
>> +
>> +	*rval = readl(regs_base + SC_DJTAG_RD_DATA_BASE_EX +
>> +					      chain_id * 0x4);
> Weird alignment. Please align to the right of the '(', using spaces as
> necessary, e.g.
>
> 	*rval = readl(regs_base + SC_DJTAG_RD_DATA_BASE_EX +
> 		      chain_id * 4);

Thanks, shall correct it and recheck in the entire series.

>> +
>> +	return 0;
>> +}
>> +
>> +/*
>> + * djtag_write_v1/v2: djtag 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
>> + * @wval:	value to register for write
>> + * @chain_id:	which sub module for read
>> + *
>> + * Return non-zero if error, else return 0.
>> + */
>> +static int djtag_write_v1(void __iomem *regs_base, u32 offset, u32 mod_sel,
>> +			  u32 mod_mask, u32 wval, int chain_id)
>> +{
>> +	int ret;
>> +
>> +	if (!(mod_mask & CHAIN_UNIT_CFG_EN)) {
>> +		pr_warn("djtag: do nothing.\n");
>> +		return 0;
>> +	}
>> +
>> +	djtag_prepare_v1(regs_base, offset, mod_sel, mod_mask);
>> +
>> +	writel(DJTAG_MSTR_W, regs_base + SC_DJTAG_MSTR_WR);
>> +	writel(wval, regs_base + SC_DJTAG_MSTR_DATA);
>> +
>> +	ret = djtag_do_operation_v1(regs_base);
>> +	if (ret) {
>> +		if (ret == EBUSY)
>> +			pr_err("djtag: %s timeout!\n", "write");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> Same comments as with the read case.
>

Ok

>> +static int djtag_write_v2(void __iomem *regs_base, u32 offset, u32 mod_sel,
>> +			  u32 mod_mask, u32 wval, int chain_id)
>> +{
>> +	int ret;
>> +
>> +	if (!(mod_mask & CHAIN_UNIT_CFG_EN_EX)) {
>> +		pr_warn("djtag: do nothing.\n");
>> +		return 0;
>> +	}
>> +
>> +	djtag_prepare_v2(regs_base, offset, mod_sel, mod_mask);
>> +
>> +	writel(DJTAG_MSTR_WR_EX
>> +			| (mod_sel << DEBUG_MODULE_SEL_SHIFT_EX)
>> +			| (mod_mask & CHAIN_UNIT_CFG_EN_EX),
>> +			regs_base + SC_DJTAG_MSTR_CFG_EX);
>> +	writel(wval, regs_base + SC_DJTAG_MSTR_DATA_EX);
>> +
>> +	ret = djtag_do_operation_v2(regs_base);
>> +	if (ret) {
>> +		if (ret == EBUSY)
>> +			pr_err("djtag: %s timeout!\n", "write");
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
> Likewise.
>
>> +/**
>> + * djtag_writel - write registers via djtag
>> + * @client: djtag client handle
>> + * @offset:	register's offset
>> + * @mod_sel:	module selection
>> + * @mod_mask:	mask to select specific modules
>> + * @val:	value to write to register
>> + *
>> + * If error return errno, otherwise return 0.
>> + */
>> +int hisi_djtag_writel(struct hisi_djtag_client *client, u32 offset,
>> +		      u32 mod_sel, u32 mod_mask, u32 val)
> The function name doesn't match the comment block above.
>

Thanks, shall correct it and recheck in this file.

>> +{
>> +	void __iomem *reg_map = client->host->sysctl_reg_map;
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	spin_lock_irqsave(&client->host->lock, flags);
>> +	ret = client->host->djtag_ops->djtag_write(reg_map, offset, mod_sel,
>> +						   mod_mask, val, 0);
>> +	if (ret)
>> +		pr_err("djtag_writel: error! ret=%d\n", ret);
>> +	spin_unlock_irqrestore(&client->host->lock, flags);
>> +
>> +	return ret;
>> +}
> Please use some temporary variables rather than a long chain of
> dereferences.
>
> AFAICT, the error here is pointless noise given the write op already
> logs an error when it return a non-zero value.
>
> So I think this can be:
>
> int hisi_djtag_writel(struct hisi_djtag_client *client, u32 offset,
> 		      u32 mod_sel, u32 mod_mask, u32 val)
> {
> 	struct hisi_djtag_host *host = client->host;
> 	struct hisi_djtag_ops *ops = host->djtag_ops;
> 	void __iomem *reg_map = host->sysctl_reg_map;
> 	unsigned long flags;
> 	int ret;
>
> 	spin_lock_irqsave(&client->host->lock, flags);
> 	ret = ops->djtag_write(reg_map, offset, mod_sel, mod_mask, val, 0);
> 	spin_unlock_irqrestore(&client->host->lock, flags);
>
> 	return ret;
> }
>
> Please consistently use the hisi_djtag_ prefix. It is confusing that
> some functions have it while others do not.
> 	

Thanks, shall modify accordingly.

>> +/**
>> + * djtag_readl - read registers via djtag
>> + * @client: djtag client handle
>> + * @offset:	register's offset
>> + * @mod_sel:	module type selection
>> + * @chain_id:	chain_id number, mostly is 0
>> + * @val:	register's value
>> + *
>> + * If error return errno, otherwise return 0.
>> + */
>> +int hisi_djtag_readl(struct hisi_djtag_client *client, u32 offset,
>> +		     u32 mod_sel, int chain_id, u32 *val)
>> +{
>> +	void __iomem *reg_map = client->host->sysctl_reg_map;
>> +	unsigned long flags;
>> +	int ret = 0;
>> +
>> +	spin_lock_irqsave(&client->host->lock, flags);
>> +	ret = client->host->djtag_ops->djtag_read(reg_map, offset, mod_sel,
>> +						  0xffff, chain_id, val);
>> +	if (ret)
>> +		pr_err("djtag_readl: error! ret=%d\n", ret);
>> +	spin_unlock_irqrestore(&client->host->lock, flags);
>> +
>> +	return ret;
>> +}
> Same comments as for hisi_djtag_writel.

Ok.

> [...]
>
>> +static struct attribute *hisi_djtag_dev_attrs[] = {
>> +	NULL,
>> +	/* modalias helps coldplug:  modprobe $(cat .../modalias) */
>> +	&dev_attr_modalias.attr,
>> +	NULL
>> +};
> Why is the first entry NULL?

its a mistake. shall remove it.

> [...]
>
>> +static struct hisi_djtag_client *hisi_djtag_verify_client(struct device *dev)
>> +{
>> +	return (dev->type == &hisi_djtag_client_type)
>> +			? to_hisi_djtag_client(dev)
>> +			: NULL;
>> +}
> s/verify/get/
>
> Why would this be called for a device which is not a djtag client?

Yes. Djtag followed i2c bus. So this code came here. Thanks for pointing 
it. Shall remove it.

> [...]
>
>> +static int hisi_djtag_device_match(struct device *dev,
>> +				   struct device_driver *drv)
>> +{
>> +	struct hisi_djtag_client *client = hisi_djtag_verify_client(dev);
>> +
>> +	if (!client)
>> +		return false;
> Does this case ever happen?

Agreeed. only for djtag client this will be called. so shall remove it.

> [...]
>
>> +	snprintf(client->name, DJTAG_CLIENT_NAME_LEN, "%s%s_%d",
>> +		 DJTAG_PREFIX, device_name, client->id);
>> +	dev_set_name(&client->dev, "%s", client->name);
> This implies that hisi_djtag_client::name is redundant.
>
> Please get rid of it, and use the name of hisi_djtag_client::dev where
> necessary.

Agreed. hisi_djtag_client::name is not needed. shall use 
hisi_djtag_client::dev.

> [...]
>
>> +static void djtag_register_devices(struct hisi_djtag_host *host)
>> +{
>> +	struct device_node *node;
>> +
>> +	if (host->of_node) {
> This is always true since the driver only probes via DT.
>
> Please get rid of this check until it becomes necessary.

Ok. shall add it when adding ACPI support.

> [...]
>
>> +static int djtag_host_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct hisi_djtag_host *host;
>> +	struct resource *res;
>> +	int rc;
> s/rc/ret/ for consistency with other code in this directory.
>
> That applies to all instances in this patch.

Ok. I shall make it consistent.

> I'm aware the xgene PMU doesn't stick to that style, and that was a
> mistake.
>
>> +	host = kzalloc(sizeof(*host), GFP_KERNEL);
>> +	if (!host)
>> +		return -ENOMEM;
>> +
>> +	if (dev->of_node) {
> Likewise this should always be the case, since the driver only probes
> via DT.
>
> Please get rid of this check until it becomes necessary.

Ok. shall add it when adding ACPI support.


> [...]
>
>> +	if (!resource_size(res)) {
>> +		dev_err(dev, "Zero reg entry!\n");
>> +		rc = -EINVAL;
>> +		goto fail;
>> +	}
> ... but any non-zero size is fine?
>
> If you want to check the size, please check it meets your minimum
> requirement.

The resource size should be 0x10000. I shall add macro and a check for it.

Thanks,
Anurup

> Thanks,
> Mark.

[toc] | [prev] | [next] | [standalone]

#1608348

FromMark Rutland <mark.rutland@arm.com>
Date2017-03-24 12:40 +0100
Message-ID<tovJv-Sg-17@gated-at.bofh.it>
In reply to#1608308
On Fri, Mar 24, 2017 at 04:16:25PM +0530, Anurup M wrote:
> Thanks for the review.
> 
> On Tuesday 21 March 2017 09:21 PM, Mark Rutland wrote:
> >On Fri, Mar 10, 2017 at 01:28:22AM -0500, Anurup M wrote:

> >>+#define SC_DJTAG_TIMEOUT_US    (100 * USEC_PER_MSEC) /* 100ms */
> >How was this value chosen?
> >
> >How likely is a timeout?
> 
> As explained in PATCH 7,
> 
> The djtag -EBUSY in hardware is a very rare scenario, and by design
> of hardware, does not occur unless there is a Chip hung situation.
> The maximum timeout possible in djtag is 30us, and hardware logic
> shall reset it, if djtag is unavailable for more than 30us.
> The timeout used in driver is 100ms to ensure that it does not fail
> in any case.

I couldn't find such an explanation in patch 7.

So that this doesn't get lost, please place a comment to this effect
above the definition of SC_DJTAG_TIMEOUT_US.

We can drop the existing "100ms" comment at the same time.

What exactly does a "chip hung situation" imply? Does that just mean
that only the djtag HW is hung, or that other parts of the chip are hung
too, such taht other things are likely to go wrong?

[...]

> >static int djtag_do_operation_v1(void __iomem *regs_base)
> >{
> >	int ret;
> >	u32 val;
> >
> >	/* start to write to djtag register */
> >	writel(DJTAG_MSTR_START_EN, regs_base + SC_DJTAG_MSTR_START_EN);
> >
> >	/* wait for the operation to complete */
> >	ret = readl_poll_timout(regs_base + SC_DJTAG_MSTR_START_EN,
> >				val, !(val & DJTAG_MSTR_EN),
> >				1, SC_DJTAG_TIMEOUT_US);
> >	
> >	if (ret)
> >		pr_warn("djtag operation timed out.\n");
> >
> >	return ret;
> >}
> >
> >Depending on how serious a timeout is, this might want to be some kind
> >of WARN variant.

From the above, I take that a hang indicates a very serious problem, so
this whould be a WARN, with a comment:

	/* 
	 * A timeout should never occur on a working system. See the
	 * definition of SC_DJTAG_TIMEOUT_US.
	 */
	WARN(ret, "djtag operation timed out.\n");

... or, if this really should never happen, and other things are likely
to go wrong were we to see this, we can BUG_ON(ret) instead, remove the
error code, and simplify all the callers.

Likewise for djtag_do_operation_v2().

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1609509

FromAnurup M <anurupvasu@gmail.com>
Date2017-03-27 08:40 +0200
Message-ID<tpwtQ-3Lz-15@gated-at.bofh.it>
In reply to#1608348

On Friday 24 March 2017 05:06 PM, Mark Rutland wrote:
>>>> +#define SC_DJTAG_TIMEOUT_US    (100 * USEC_PER_MSEC) /* 100ms */
>>> > >How was this value chosen?
>>> > >
>>> > >How likely is a timeout?
>> >
>> >As explained in PATCH 7,
>> >
>> >The djtag -EBUSY in hardware is a very rare scenario, and by design
>> >of hardware, does not occur unless there is a Chip hung situation.
>> >The maximum timeout possible in djtag is 30us, and hardware logic
>> >shall reset it, if djtag is unavailable for more than 30us.
>> >The timeout used in driver is 100ms to ensure that it does not fail
>> >in any case.
> I couldn't find such an explanation in patch 7.

Sorry, I intent to mean that it was explained in the reply to PATCH 7 
comments.

> So that this doesn't get lost, please place a comment to this effect
> above the definition of SC_DJTAG_TIMEOUT_US.
>
> We can drop the existing "100ms" comment at the same time.
>
> What exactly does a "chip hung situation" imply? Does that just mean
> that only the djtag HW is hung, or that other parts of the chip are hung
> too, such taht other things are likely to go wrong?

Yes it is not that only djtag HW is hung, but it means there is some 
serious irrecoverable
condition in chip, where other things are also likely to go wrong.
Shall add comments above definition of SC_DJTAG_TIMEOUT_US.

> [...]
>
>>> 	/* wait for the operation to complete */
>>> > >	ret = readl_poll_timout(regs_base + SC_DJTAG_MSTR_START_EN,
>>> > >				val, !(val & DJTAG_MSTR_EN),
>>> > >				1, SC_DJTAG_TIMEOUT_US);
>>> > >	
>>> > >	if (ret)
>>> > >		pr_warn("djtag operation timed out.\n");
>>> > >
>>> > >	return ret;
>>> > >}
>>> > >
>>> > >Depending on how serious a timeout is, this might want to be some kind
>>> > >of WARN variant.
>  From the above, I take that a hang indicates a very serious problem, so
> this whould be a WARN, with a comment:
>
> 	/*
> 	 * A timeout should never occur on a working system. See the
> 	 * definition of SC_DJTAG_TIMEOUT_US.
> 	 */
> 	WARN(ret, "djtag operation timed out.\n");
>
> ... or, if this really should never happen, and other things are likely
> to go wrong were we to see this, we can BUG_ON(ret) instead, remove the
> error code, and simplify all the callers.
>
> Likewise for djtag_do_operation_v2().

Ok. As this should never happen by design of hardware, it is a serious 
problem. So shall
add BUG_ON and simplify all callers.

Thanks,
Anurup

> Thanks,
> Mark.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web