Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752148AbcKHHii (ORCPT ); Tue, 8 Nov 2016 02:38:38 -0500 Received: from mail-pf0-f194.google.com ([209.85.192.194]:33712 "EHLO mail-pf0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750938AbcKHHih (ORCPT ); Tue, 8 Nov 2016 02:38:37 -0500 Subject: Re: [PATCH v1 03/11] drivers: soc: hisi: Add support for Hisilicon Djtag driver To: Arnd Bergmann , 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> <58217871.6050609@huawei.com> Cc: Tan Xiaojun , 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 From: Anurup M Message-ID: <582180F7.5060100@gmail.com> Date: Tue, 8 Nov 2016 13:08:31 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <58217871.6050609@huawei.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6194 Lines: 182 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 >>> >>> 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 >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#include >> 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. Shall use non-relaxed version and make it consistent. >>> + /* 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. >>> +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? On some chips like hip06, the djtag version is different for IO die. Thanks, Anurup >> >> 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 >> >> >> . >> >