Re: [PATCH 8/8] clk: tegra: Add EMC clock driver

[Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

Quoting Stephen Warren (2014-07-31 12:53:54)
> On 07/31/2014 01:06 PM, Mike Turquette wrote:
> > Quoting Thierry Reding (2014-07-30 02:34:57)
> >> On Tue, Jul 29, 2014 at 04:14:44PM -0600, Stephen Warren wrote:
> >>> On 07/29/2014 02:19 PM, Mike Turquette wrote:
> >>>> Quoting Mikko Perttunen (2014-07-29 01:47:35)
> >>>>> On 22/07/14 19:57, Stephen Warren wrote:
> >>>>>> On 07/11/2014 08:18 AM, Mikko Perttunen wrote:
> >>>>>>> +static int emc_debug_rate_set(void *data, u64 rate)
> >>>>>>> +{
> >>>>>>> +    struct tegra_emc *tegra = data;
> >>>>>>> +
> >>>>>>> +    return clk_set_rate(tegra->hw.clk, rate);
> >>>>>>> +}
> >>>>>>> +
> >>>>>>> +DEFINE_SIMPLE_ATTRIBUTE(emc_debug_rate_fops, emc_debug_rate_get,
> >>>>>>> +                    emc_debug_rate_set, "%lld\n");
> >>>>>>
> >>>>>> I think the rate can already be obtained through
> >>>>>> ...debug/clock/clock_summary. I'm not sure about changing the rate, but
> >>>>>> shouldn't that be a feature of the common clock core, not individual
> >>>>>> drivers?
> >>>>>
> >>>>> The core doesn't allow writing to the rate debugfs files, so this is the
> >>>>> only way to trigger an EMC clock change for now. I agree that the core
> >>>>> might be a better place. I don't know if there are any philosophical
> >>>>> objections to that. I'd like to keep this in until a possible core
> >>>>> feature addition. Mike, any comments?
> >>>>
> >>>> Yes, there is a philosophical rejection to exposing rate-change knobs to
> >>>> userspace through debugfs. These can and will ship in real products
> >>>> (typically Android) with lots of nasty userspace hacks, and also
> >>>> represent pretty dangerous things to expose to userspace. I have always
> >>>> maintained that such knobs should remain out of tree or, with the advent
> >>>> of the custom debugfs entries, should be burden of the clock drivers.
> >>>
> >>> That argument seems a bit inconsistent.
> >>>
> >>> I can see the argument to disallow code that lets user-space fiddle with
> >>> clocks. However, if that argument holds, then surely it must apply to either
> >>> the clock core *or* a clock driver; the end effect of allowing the code in
> >
> > Stephen,
> >
> > You meant to say, "it must apply to both the clock core and a clock
> > driver"? I agree.
> 
> Sure; s/either/both/ in what I said.
> 
> >>> either place is that people will be able to implement the user-space hacks
> >>> you want to avoid. Yet, if we allow the code because it's a useful debug
> >>> tool, then surely it should be in the clock core so we don't implement it
> >>> redundantly in each clock driver.
> >
> > I don't want it anywhere to be honest. Read-only debugfs stuff is great
> > and I'm happy to merge it. I have repeatedly NAK'd any attempt to get
> > the userspace rate-change stuff merged into the core.
> >
> > Recently we have the ability to assign custom debugfs entries that are
> > specific to the clock driver. I'm trying to find the right balance
> > between giving the clock driver authors the right amount of autonomy to
> > implement what they need while trying to keep the crazy out of the
> > kernel. Maybe in this case I should stick to my guns and NAK this patch.
> 
> If someone implements the same thing in some downstream/product kernel, 
> and it can't be upstreamed, I'd argue they should still do that in the 
> clock core rather than individual clock drivers, since they'll probably 
> want the feature for multiple clocks. I don't think  the per-clock 
> debugfs hook is useful in this case, although I can certainly imagine 
> other read-only per-clock debug files could be useful.

That is sensible, and all the more reason that this patch shouldn't
implement the rate-change feature within the clock driver. So consider
it NAK'd.

Also I agree that the per-clock debugfs entries are very useful for RO
operations, especially exposing how registers are programmed or other
relevant data.

Regards,
Mike