2014-07-11 16:01:41

by Mikko Perttunen

[permalink] [raw]

Subject: Re: [PATCH 5/8] of: Add Tegra124 EMC bindings

On 07/11/2014 05:51 PM, Thierry Reding wrote:
> On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote:
>> Add binding documentation for the nvidia,tegra124-emc device tree
>> node.
>>
>> Signed-off-by: Mikko Perttunen <[email protected]>
>> ---
>> .../bindings/memory-controllers/tegra-emc.txt | 42 ++++++++++++++++++++++
>> 1 file changed, 42 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt
>> new file mode 100644
>> index 0000000..2dde17e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/memory-controllers/tegra-emc.txt
>> @@ -0,0 +1,42 @@
>> +Tegra124 SoC EMC controller
>> +
>> +Required properties :
>> +- compatible : "nvidia,tegra124-emc".
>> +- reg : Should contain 1 or 2 entries:
>> + - EMC register set
>> + - MC register set : Required only if no node with
>> + 'compatible = "nvidia,tegra124-mc"' exists. The MC register set
>> + is first read from the MC node. If it doesn't exist, it is read
>> + from this property.
>
> No can do. Memory regions shouldn't be shared between drivers like this.
> It makes it impossible to ensure that they don't stump on each others'
> toes.

In this case, all the registers that will be written are such that the
MC driver will never need to write them. They are shadowed registers,
meaning that all writes are stored and are only effective starting from
the next time the EMC rate change state machine is activated, so writing
them from anywhere except than the EMC driver would be pointless.

I can find two users of these registers in downstream:
1) mc.c saves and loads them on suspend/restore (I don't know why, that
shouldn't do anything. They will be overridden anyway during the next
EMC rate change).
2) tegra12x_la.c reads MC_EMEM_ARB_MISC0 during a core_initcall to
calculate a value which it then writes to a register that is also
shadowed and that is part of downstream burst registers so that doesn't
do anything either.

The reason I implemented two ways to specify the MC register area was
that this could be merged before an MC driver and retain
backwards-compatibility after the MC driver arrives.

If this is not acceptable, we can certainly wait for the MC driver to be
merged first. (Although with the general rate of things, I hope I won't
be back at school at that point..) I assume that this is blocked on the
IOMMU bindings discussion? In that case, there are several options: the
MC driver could have its own tables for each EMC rate or we could just
make the EMC tables global (i.e. not under the EMC node). In any case,
the MC driver would need to implement a function that would just write
these values but be guaranteed to not do anything else, since that could
cause nasty things during the EMC rate change sequence.

Yet another option is to just not write to these registers at all. In my
tests, that would entail a 20-25% penalty to memory throughput for most
timings.

>
> One possibility to make this work is to export global functions from the
> memory controller driver that this driver can call into. Perhaps if you
> want you can be extra explicit by linking them in DT, like this:
>
> mc: memory-controller@0,70019000 {
> compatible = "nvidia,tegra124-mc";
> reg = <0x0 0x70019000 0x0 0x00001000>;
> ...
> };
>
> memory-controller@0,7001b000 {
> compatible = "nvidia,tegra124-emc";
> reg = <0x0 0x7001b000 0x0 0x00001000>;
> memory-controller = <&mc>;
> ...
> };
>
> But I think it's safe enough to assume that there will only be a single
> memory controller/EMC pair in the device.
>
> Thierry
>

2014-07-11 16:43:40

by Andrew Bresticker

[permalink] [raw]

Subject: Re: [PATCH 5/8] of: Add Tegra124 EMC bindings

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 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.

We could always taint the kernel if the feature is used. Admittedly that
wouldn't stop people using the feature as a hack in Android/product
kernels, but at least nobody would have to unknowingly debug problems
due to such manipulation, in the context of an upstream kernel.

2014-07-30 09:35:05

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