Re: [PATCH v3 3/5] coresight: add support for debug module

[Leo Yan <leo.yan@linaro.org>]

On Wed, Mar 22, 2017 at 05:25:50PM +0000, Sudeep Holla wrote:
> 
> 
> On 22/03/17 17:09, Suzuki K Poulose wrote:
> > On 22/03/17 16:17, Sudeep Holla wrote:
> 
> [...]
> 
> >> 
> >> Point taken. So we could just specify that all necessary power
> >> domains need to be on for proper functionality for this feature and
> >> that it's highly platform specific instead of mixing cpu/cluster
> >> idle details here.
> >> 
> >>> The key point is that the caveat in using this driver is that
> >>> the power management has to be considered on a platform specific
> >>> basis before it is configured; and appropriate actions may be
> >>> needed for it to work correctly. Without this then the driver
> >>> could cause more issues than it debugs. A user selecting this
> >>> _must_ be told about these issues
> >>> 
> > 
> > So given all the possible caveats, I think we :
> > 
> > 1) Shouldn't enable the driver by default at runtime even if it is 
> > built-in. 
> > 2) Should provide mechanisms to turn it on at boot (via
> > kernel commandline) or anytime later (via sysfs), which kind of puts
> > the responsibility back on the user : "You know what you are doing". 
> > 3) Shouldn't turn the driver on based on "nohlt" which the user
> > could use it for some other purposes, without explicit intention of
> > turning this driver on).
> > 4) Should document the fact that, on some
> > platforms, the user may have to disable CPUidle explicitly to get the
> > driver working. But let us not make it the default. The user with a
> > not so ideal platform could add "nohlt" and get it working.
> > 
> 
> Agreed on all points and well summarized.
> I would like to highlight (3) and (4) as it needs to be well understood.
> 
> "nohlt" has a *different* meaning already, so using that in this
> driver for something else is simple wrong as it affects the system in
> unintended ways. And yes if user (mis)uses it to get things working,
> it's fine but shouldn't be recommended way.

Understand this point.

I will try to use general way to constraint CPUIdle like other
drivers.

Thanks all for these good suggestions :)

Thanks,
Leo Yan