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

[Mikko Perttunen]

On 14/07/14 11:15, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Mon, Jul 14, 2014 at 10:55:51AM +0300, Mikko Perttunen wrote:
> > On 11/07/14 19:01, Mikko Perttunen wrote:
> > > On 07/11/2014 05:51 PM, Thierry Reding wrote:
> > > > On Fri, Jul 11, 2014 at 05:18:30PM +0300, Mikko Perttunen wrote:
> > > > > ...
> > > > ...
> > >
> > > 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.
> >
> > Having taken another look at the code, I don't think the MC driver could do
> > anything that bad. There are also two other places where the EMC driver
> > needs to read MC registers: Inside the sequence, it reads a register but
> > discards its contents. According to comments, this acts as a memory barrier,
> > probably for the preceding step that writes into MC memory. If the register
> > writes are moved to the MC driver, it could also handle that. In another
> > place it reads the number of RAM modules from a MC register. The MC driver
> > could export this as another function.
>
> Exporting this functionality from the MC driver is the right thing to do
> in my opinion.

Ok, let's do that then. Do you think I could make a bare-bones MC driver 
to support the EMC driver before your MC driver with IOMMU/LA is ready? 
Can the MC device tree node be stabilized yet? Of course, if things go 
well, that concern might turn out to be unnecessary.

Thanks, Mikko.

> > That said, I still suspect it might not be worth it to divide this between
> > two drivers.
>
> If we don't separate, then we make it easy for people to break things in
> the future. Both drivers may not be accessing the same registers now,
> but if there's no barrier in place to actively enforce this split, then
> it's only a matter of time before it gets broken. A typical way to
> ensure this is done properly is via request_resource() (called by the
> devm_ioremap_resource() function). That will fail if two drivers try to
> use the same memory region, and with good reason.
>
> Thierry
>
> * Unknown Key
> * 0x7F3EB3A1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/