From: Arnd Bergmann <arnd@arndb.de>
To: Anurup M <anurupvasu@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org,
        Tan Xiaojun <tanxiaojun@huawei.com>, anurup.m@huawei.com,
        linux-kernel@vger.kernel.org, mark.rutland@arm.com,
        shyju.pv@huawei.com, gabriele.paoloni@huawei.com,
        john.garry@huawei.com, will.deacon@arm.com, linuxarm@huawei.com,
        xuwei5@hisilicon.com, zhangshaokun@hisilicon.com,
        sanil.kumar@hisilicon.com, shiju.jose@huawei.com
Subject: Re: [PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver
Date: Tue, 08 Nov 2016 12:43:41 +0100
Message-ID: <22120038.dbaYpsQoUH@wuerfel>
User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; )
In-Reply-To: <582180F7.5060100@gmail.com>
References: <1478101374-18778-1-git-send-email-anurup.m@huawei.com> <58217871.6050609@huawei.com> <582180F7.5060100@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2535
Lines: 65

On Tuesday, November 8, 2016 1:08:31 PM CET Anurup M wrote:
 
> On Tuesday 08 November 2016 12:32 PM, Tan Xiaojun wrote:
> > On 2016/11/7 21:26, Arnd Bergmann wrote:
> >> On Wednesday, November 2, 2016 11:42:46 AM CET Anurup M wrote:
> >>> From: Tan Xiaojun <tanxiaojun@huawei.com>
> >>> +	/* 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.
> >>
> > Yes, this is not reentrant.
> The read/write functions shall also be called from irq handler (for 
> handling counter overflow).
> So need to use udelay calls. Shall Document it in v2.

Ok.

> >>> +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?
> On some chips like hip06, the djtag version is different for IO die.

In what way? The driver doesn't seem to care about the difference.

	Arnd