From: Tan Xiaojun <tanxiaojun@huawei.com>
To: Arnd Bergmann <arnd@arndb.de>, <linux-arm-kernel@lists.infradead.org>
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>,
<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, 8 Nov 2016 15:02:09 +0800 [thread overview]
Message-ID: <58217871.6050609@huawei.com> (raw)
In-Reply-To: <2127881.qmAR1XgW9F@wuerfel>
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>
>>
>> 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.
>
Sorry for that. We will fix it.
>> 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.
>
OK. For now, this suggestion sounds good.
>> +
>> +#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.
>
>
OK. Sorry for that.
>> +DEFINE_IDR(djtag_hosts_idr);
>
> make this static
>
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);
>> +}
>
> 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.
>
Yes, it is our mistake.
>> + /* 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.
>> +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 *)'.
>
OK. We will consider it.
Thanks.
Xiaojun.
>> +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
>
>
> .
>
next prev parent reply other threads:[~2016-11-08 7:02 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-02 15:42 [PATCH v1 00/11] perf: arm64: Support for Hisilicon SoC Hardware event counters Anurup M
2016-11-02 15:42 ` [PATCH v1 01/11] arm64: MAINTAINERS: hisi: Add hisilicon SoC PMU support Anurup M
2016-11-02 15:42 ` [PATCH v1 02/11] dt-bindings: hisi: Add Hisilicon HiP05/06/07 Sysctrl and Djtag dts bindings Anurup M
2016-11-02 15:42 ` [PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver Anurup M
2016-11-03 1:59 ` kbuild test robot
2016-11-07 13:26 ` Arnd Bergmann
2016-11-07 14:15 ` John Garry
2016-11-07 20:08 ` Arnd Bergmann
2016-11-08 11:23 ` John Garry
2016-11-08 11:45 ` Arnd Bergmann
2016-11-08 13:49 ` John Garry
2016-11-08 15:10 ` Arnd Bergmann
2016-11-08 15:17 ` John Garry
2016-11-09 10:44 ` Anurup M
2016-11-08 15:51 ` Anurup M
2016-11-09 9:06 ` John Garry
2016-11-11 10:35 ` Anurup M
2016-11-08 7:02 ` Tan Xiaojun [this message]
2016-11-08 7:38 ` Anurup M
2016-11-08 11:43 ` Arnd Bergmann
2016-11-08 13:46 ` Anurup M
2016-11-08 15:08 ` Arnd Bergmann
2016-11-09 4:28 ` Anurup M
2016-11-09 21:40 ` Arnd Bergmann
2016-11-11 10:23 ` Anurup M
2016-11-02 15:42 ` [PATCH v1 04/11] Documentation: perf: hisi: Documentation for HIP05/06/07 PMU event counting Anurup M
2016-11-02 15:42 ` [PATCH v1 05/11] dt-bindings: perf: hisi: Add Devicetree bindings for Hisilicon SoC PMU Anurup M
2016-11-02 15:42 ` [PATCH v1 06/11] perf: hisi: Update Kconfig for Hisilicon PMU support Anurup M
2016-11-02 15:42 ` [PATCH v1 07/11] perf: hisi: Add support for Hisilicon SoC event counters Anurup M
2016-11-02 15:42 ` [PATCH v1 08/11] perf: hisi: Add sysfs attributes for L3 cache(L3C) PMU Anurup M
2016-11-02 15:42 ` [PATCH v1 09/11] perf: hisi: Miscellanous node(MN) event counting in perf Anurup M
2016-11-02 15:42 ` [PATCH v1 10/11] perf: hisi: Support for Hisilicon DDRC PMU Anurup M
2016-11-02 15:42 ` [PATCH v1 11/11] dts: arm64: hip06: Add Hisilicon SoC PMU support Anurup M
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=58217871.6050609@huawei.com \
--to=tanxiaojun@huawei.com \
--cc=anurup.m@huawei.com \
--cc=anurupvasu@gmail.com \
--cc=arnd@arndb.de \
--cc=gabriele.paoloni@huawei.com \
--cc=john.garry@huawei.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxarm@huawei.com \
--cc=mark.rutland@arm.com \
--cc=sanil.kumar@hisilicon.com \
--cc=shiju.jose@huawei.com \
--cc=shyju.pv@huawei.com \
--cc=will.deacon@arm.com \
--cc=xuwei5@hisilicon.com \
--cc=zhangshaokun@hisilicon.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).