Subject: Re: [PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon
 Djtag driver
To: Arnd Bergmann <arnd@arndb.de>, <linux-arm-kernel@lists.infradead.org>
References: <1478101374-18778-1-git-send-email-anurup.m@huawei.com>
 <1478101374-18778-4-git-send-email-anurup.m@huawei.com>
 <2127881.qmAR1XgW9F@wuerfel>
CC: Anurup M <anurupvasu@gmail.com>, <anurup.m@huawei.com>,
        <linux-kernel@vger.kernel.org>, <mark.rutland@arm.com>,
        <shyju.pv@huawei.com>, <gabriele.paoloni@huawei.com>,
        <will.deacon@arm.com>, <linuxarm@huawei.com>, <xuwei5@hisilicon.com>,
        <zhangshaokun@hisilicon.com>, <sanil.kumar@hisilicon.com>,
        <tanxiaojun@huawei.com>, <shiju.jose@huawei.com>
From: John Garry <john.garry@huawei.com>
Message-ID: <2a8dc40f-be75-b010-3ec7-6912f02e3c90@huawei.com>
Date: Mon, 7 Nov 2016 14:15:10 +0000
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101
 Thunderbird/45.3.0
MIME-Version: 1.0
In-Reply-To: <2127881.qmAR1XgW9F@wuerfel>
Content-Type: text/plain; charset="windows-1252"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 6213
Lines: 193

On 07/11/2016 13:26, Arnd Bergmann wrote:
> On Wednesday, November 2, 2016 11:42:46 AM CET 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.
>
> The formatting of the text seems odd, please remove the leading spaces.
>

My only response is at the bottom (I will not snip the code for early 
referencing convenience).

Anurup/Tan can respond to the rest.

>>  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 +++
>
> Do you expect other drivers to be added that reference this interface?
> If not, or if you are unsure, just put all of it under drivers/perf
> so we don't introduce a global API that has only one user.
>
>> +
>> +#include <linux/bitops.h>
>> +#include <linux/init.h>
>> +#include <linux/list.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +
>> +#include <asm-generic/delay.h>
>
> Never include files from asm-generic directly except from
> an architecture specific asm/*.h header file.
>
>
>> +DEFINE_IDR(djtag_hosts_idr);
>
> make this static
>
>> +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);
>> +}
>
> This looks like an odd combination of interfaces.
> Why can the reads be "relaxed" when the writes can not?
>
> Generally speaking, I'd advise to always use non-relaxed accessors
> unless there is a strong performance reason, and in that case there
> should be a comment explaining the use at each of the callers
> of a relaxed accessor.
>
>> +	/* ensure the djtag operation is done */
>> +	do {
>> +		djtag_read32_relaxed(regs_base, SC_DJTAG_MSTR_START_EN_EX, &rd);
>> +
>> +		if (!(rd & DJTAG_MSTR_START_EN_EX))
>> +			break;
>> +
>> +		udelay(1);
>> +	} while (timeout--);
>
> This one is obviously not performance critical at all, so use a non-relaxed
> accessor. Same for the other two in this function.
>
> Are these functions ever called from atomic context? If yes, please document
> from what context they can be called, otherwise please consider changing
> the udelay calls into sleeping waits.
>
>> +int hisi_djtag_writel(struct hisi_djtag_client *client, u32 offset, u32 mod_sel,
>> +							u32 mod_mask, 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_readwrite(reg_map, offset, mod_sel, mod_mask,
>> +					true, val, 0, NULL);
>> +	if (ret)
>> +		pr_err("djtag_writel: error! ret=%d\n", ret);
>> +	spin_unlock_irqrestore(&client->host->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(hisi_djtag_writel);
>
> That would of course imply changing the spinlock to a mutex here as well.
>
>> +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 },
>> +	{},
>> +};
>
> If these are backwards compatible, just mark them as compatible in DT,
> e.g. hip06 can use
>
> 	compatible = "hisilicon,hip06-cpu-djtag-v1", "hisilicon,hip05-cpu-djtag-v1";
>
> so you can tell the difference if you need to, but the driver only has to
> list the oldest one here.
>
> What is the difference between the cpu and io djtag interfaces?
>
> I think you can also drop the '(void *)'.
>
>> +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);
>> +	}
>> +}
>
> Can you explain your thoughts behind creating a new bus type
> and adding the child devices manually rather than using
> platform_device structures with of_platform_populate()?
>
> Do you expect to see other implementations of this bus type
> with incompatible bus drivers?
>
> 	Arnd
>

Hi Arnd,

The new bus type tries to model the djtag in a similar way to I2C/USB 
driver arch, where we have a host bus adapter and child devices attached 
to the bus. The child devices are bus driver devices and have bus 
addresses. We think of the djtag as a separate bus, so we are modelling 
it as such.

The bus driver offers a simple host interface for clients to read/write 
to the djtag bus: bus accesses are hidden from the client, the host 
drives the bus.

 > Do you expect to see other implementations of this bus type
 > with incompatible bus drivers?

Maybe, not sure.

Cheers,
John

>
> .
>