From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753068AbdCOQpV (ORCPT ); Wed, 15 Mar 2017 12:45:21 -0400 Received: from foss.arm.com ([217.140.101.70]:49588 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751785AbdCOQor (ORCPT ); Wed, 15 Mar 2017 12:44:47 -0400 Subject: Re: [v3 3/5] coresight: add support for debug module To: Mathieu Poirier References: <1488520809-31670-4-git-send-email-leo.yan@linaro.org> <011fdac0-59bf-b539-2de3-0b59a41bc922@arm.com> <20170309175915.GA964@leoy-linaro> <3f27efee-3b63-81fd-eb96-73fd7e6f5e92@arm.com> <20170313165600.GB32431@linaro.org> Cc: Leo Yan , Rob Herring , Mark Rutland , Wei Xu , Catalin Marinas , Will Deacon , Michael Turquette , Stephen Boyd , John Stultz , Guodong Xu , Haojian Zhuang , Greg Kroah-Hartman , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-clk@vger.kernel.org, mike.leach@linaro.org, Sudeep Holla From: Suzuki K Poulose Message-ID: <516f8989-4dde-2686-d549-0761feb14d1b@arm.com> Date: Wed, 15 Mar 2017 16:44:37 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.6.0 MIME-Version: 1.0 In-Reply-To: <20170313165600.GB32431@linaro.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 13/03/17 16:56, Mathieu Poirier wrote: > On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote: >>>>> + >>>>> + put_online_cpus(); >>>>> + >>>>> + if (!debug_count++) >>>>> + atomic_notifier_chain_register(&panic_notifier_list, >>>>> + &debug_notifier); >>>>> + >>>> >>>>> + sprintf(buf, (char *)id->data, drvdata->cpu); >>>>> + dev_info(dev, "%s initialized\n", buf); >>>> >>>> This could simply be : >>>> dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu); >>>> >>>> and get rid of the static string and the buffer, see below. >> >> Also we need pm_runtime_put() here to balance the pm_runtime_get_ from AMBA >> device probe. > > Good point. > >> More on that below. >> >>>> >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static struct amba_id debug_ids[] = { >>>>> + { /* Debug for Cortex-A53 */ >>>>> + .id = 0x000bbd03, >>>>> + .mask = 0x000fffff, >>>> >>>> ... >>>> >>>>> + .data = "Coresight debug-CPU%d", >>>> >>>> I think this is pointless, as the debug area we are interested in is always associated >>>> with a CPU, we could as well figure out what to print from the drvdata->cpu above. >>> >>> I prefer to follow your suggestion for upper two comments; but I'd like >>> check with Mathieu, due I followed up Mathieu's suggestion to write >>> current code. >> >> Btw, I don't see any PM calls to make sure the power domain (at least the debug domain) >> is up, which could cause problems with accesses to some of these registers (leave alone the >> ones in CPU power domain), especially the EDPRSR. We could also do pm_runtime_get on the >> CPU's power domain, if the CPU is online, before we access the pcsr. > > I thought about PM runtime operations a little while back but wondered if it is > really a good thing to have them around. When this code is called the system > has crashed and as such making PM runtimes call isn't a good idea. You are right. It is not safe to make such calls when we have crashed. The other side effect is, if we don't have the debug power domain up, we could possibly hang the system and prevent other registered notifiers from running, which doesn't sound good either. > > One thing we could do is _not_ call pm_runtime_put() at the end of the probe() > operation. That way we wouldn't have to mess around with PM runtime operations > on an unstable system. This, of course, is costly in terms of power consumption > but the system is under test/debug anyway. May be control the behavior via kernel command line ? Something like coresight_debug={on or 1} or even use the "nohlt" ? Suzuki From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suzuki K Poulose Subject: Re: [v3 3/5] coresight: add support for debug module Date: Wed, 15 Mar 2017 16:44:37 +0000 Message-ID: <516f8989-4dde-2686-d549-0761feb14d1b@arm.com> References: <1488520809-31670-4-git-send-email-leo.yan@linaro.org> <011fdac0-59bf-b539-2de3-0b59a41bc922@arm.com> <20170309175915.GA964@leoy-linaro> <3f27efee-3b63-81fd-eb96-73fd7e6f5e92@arm.com> <20170313165600.GB32431@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170313165600.GB32431@linaro.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Mathieu Poirier Cc: Mark Rutland , devicetree@vger.kernel.org, Guodong Xu , Greg Kroah-Hartman , Catalin Marinas , Michael Turquette , Sudeep Holla , Will Deacon , linux-kernel@vger.kernel.org, Wei Xu , linux-clk@vger.kernel.org, Rob Herring , John Stultz , Haojian Zhuang , Leo Yan , Stephen Boyd , linux-arm-kernel@lists.infradead.org, mike.leach@linaro.org List-Id: devicetree@vger.kernel.org On 13/03/17 16:56, Mathieu Poirier wrote: > On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote: >>>>> + >>>>> + put_online_cpus(); >>>>> + >>>>> + if (!debug_count++) >>>>> + atomic_notifier_chain_register(&panic_notifier_list, >>>>> + &debug_notifier); >>>>> + >>>> >>>>> + sprintf(buf, (char *)id->data, drvdata->cpu); >>>>> + dev_info(dev, "%s initialized\n", buf); >>>> >>>> This could simply be : >>>> dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu); >>>> >>>> and get rid of the static string and the buffer, see below. >> >> Also we need pm_runtime_put() here to balance the pm_runtime_get_ from AMBA >> device probe. > > Good point. > >> More on that below. >> >>>> >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static struct amba_id debug_ids[] = { >>>>> + { /* Debug for Cortex-A53 */ >>>>> + .id = 0x000bbd03, >>>>> + .mask = 0x000fffff, >>>> >>>> ... >>>> >>>>> + .data = "Coresight debug-CPU%d", >>>> >>>> I think this is pointless, as the debug area we are interested in is always associated >>>> with a CPU, we could as well figure out what to print from the drvdata->cpu above. >>> >>> I prefer to follow your suggestion for upper two comments; but I'd like >>> check with Mathieu, due I followed up Mathieu's suggestion to write >>> current code. >> >> Btw, I don't see any PM calls to make sure the power domain (at least the debug domain) >> is up, which could cause problems with accesses to some of these registers (leave alone the >> ones in CPU power domain), especially the EDPRSR. We could also do pm_runtime_get on the >> CPU's power domain, if the CPU is online, before we access the pcsr. > > I thought about PM runtime operations a little while back but wondered if it is > really a good thing to have them around. When this code is called the system > has crashed and as such making PM runtimes call isn't a good idea. You are right. It is not safe to make such calls when we have crashed. The other side effect is, if we don't have the debug power domain up, we could possibly hang the system and prevent other registered notifiers from running, which doesn't sound good either. > > One thing we could do is _not_ call pm_runtime_put() at the end of the probe() > operation. That way we wouldn't have to mess around with PM runtime operations > on an unstable system. This, of course, is costly in terms of power consumption > but the system is under test/debug anyway. May be control the behavior via kernel command line ? Something like coresight_debug={on or 1} or even use the "nohlt" ? Suzuki From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suzuki.Poulose@arm.com (Suzuki K Poulose) Date: Wed, 15 Mar 2017 16:44:37 +0000 Subject: [v3 3/5] coresight: add support for debug module In-Reply-To: <20170313165600.GB32431@linaro.org> References: <1488520809-31670-4-git-send-email-leo.yan@linaro.org> <011fdac0-59bf-b539-2de3-0b59a41bc922@arm.com> <20170309175915.GA964@leoy-linaro> <3f27efee-3b63-81fd-eb96-73fd7e6f5e92@arm.com> <20170313165600.GB32431@linaro.org> Message-ID: <516f8989-4dde-2686-d549-0761feb14d1b@arm.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 13/03/17 16:56, Mathieu Poirier wrote: > On Fri, Mar 10, 2017 at 02:29:53PM +0000, Suzuki K Poulose wrote: >>>>> + >>>>> + put_online_cpus(); >>>>> + >>>>> + if (!debug_count++) >>>>> + atomic_notifier_chain_register(&panic_notifier_list, >>>>> + &debug_notifier); >>>>> + >>>> >>>>> + sprintf(buf, (char *)id->data, drvdata->cpu); >>>>> + dev_info(dev, "%s initialized\n", buf); >>>> >>>> This could simply be : >>>> dev_info(dev, "Coresight debug-CPU%d initialized\n", drvdata->cpu); >>>> >>>> and get rid of the static string and the buffer, see below. >> >> Also we need pm_runtime_put() here to balance the pm_runtime_get_ from AMBA >> device probe. > > Good point. > >> More on that below. >> >>>> >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static struct amba_id debug_ids[] = { >>>>> + { /* Debug for Cortex-A53 */ >>>>> + .id = 0x000bbd03, >>>>> + .mask = 0x000fffff, >>>> >>>> ... >>>> >>>>> + .data = "Coresight debug-CPU%d", >>>> >>>> I think this is pointless, as the debug area we are interested in is always associated >>>> with a CPU, we could as well figure out what to print from the drvdata->cpu above. >>> >>> I prefer to follow your suggestion for upper two comments; but I'd like >>> check with Mathieu, due I followed up Mathieu's suggestion to write >>> current code. >> >> Btw, I don't see any PM calls to make sure the power domain (at least the debug domain) >> is up, which could cause problems with accesses to some of these registers (leave alone the >> ones in CPU power domain), especially the EDPRSR. We could also do pm_runtime_get on the >> CPU's power domain, if the CPU is online, before we access the pcsr. > > I thought about PM runtime operations a little while back but wondered if it is > really a good thing to have them around. When this code is called the system > has crashed and as such making PM runtimes call isn't a good idea. You are right. It is not safe to make such calls when we have crashed. The other side effect is, if we don't have the debug power domain up, we could possibly hang the system and prevent other registered notifiers from running, which doesn't sound good either. > > One thing we could do is _not_ call pm_runtime_put() at the end of the probe() > operation. That way we wouldn't have to mess around with PM runtime operations > on an unstable system. This, of course, is costly in terms of power consumption > but the system is under test/debug anyway. May be control the behavior via kernel command line ? Something like coresight_debug={on or 1} or even use the "nohlt" ? Suzuki