Re: [PATCH v2] MTD: Onenand: Add device tree support for samsungonenand

From: Mark Rutland
Date: Thu Sep 26 2013 - 06:59:29 EST


On Tue, Sep 24, 2013 at 02:00:01PM +0100, Tomasz Figa wrote:
> Hi Mateusz, Mark,

Hi,

>
> On Monday 23 of September 2013 15:08:23 Mark Rutland wrote:
> > On Mon, Sep 23, 2013 at 02:06:48PM +0100, Mateusz Krawczuk wrote:
> > > +
> > > +Required properties:
> > > + - compatible: value should be either of the following.
> > > + (a) "samsung,s3c6400-onenand",
> > > + for onenand controller compatible with s3c6400.
> > > + (b) "samsung,s3c6410-onenand",
> > > + for onenand controller compatible with s3c6410.
> > > + (c) "samsung,s5pc100-onenand",
> > > + for onenand controller compatible with s5pc100.
> > > + (d) "samsung,s5pc110-onenand",
> > > + for s5pc100-like onenand controller used on s5pc110 which supports DMA.
> > > +
> >
> > As I asked on the last posting, what are the differences between these
> > implementations?
>
> (d) is a completely different controller than (a), (b) and (c). They
> have completely different register layouts. Also see below.

Ok.

>
> > > +Required properties for s5pc110:
> > > +
> > > + - reg: the offset and length of the control registers. First region describes
> > > + OneNAND interface, second control registers.
> >
> > Also, the complete reg description is a bit confusing. How about
> > something like:
> >
> > - reg: two register specifiers:
> > [0] - The OneNAND interface
> > [1] - The control registers
>
> This looks much better, although I wouldn't be afraid of using words
> instead of symbols to write this as follows instead:
>
> - reg: two memory mapped register regions:
> - first entry: memory mapped OneNAND chip,
> - second entry: control registers.

I'm also happy with that.

>
> But... The controller in theory supports more than one OneNAND chip, each
> mapped at different, so I'd suggest reordering the entries:
>
> - reg: memory mapped register regions:
> - first entry: control registers.
> - second entry: memory mapped OneNAND chip 0,
> - ...
> - Nth entry: memory mapped OneNAND chip (N-2).

I agree on the reordering, hence the questions I had below.

>
> > Do we expect future OneNAND devices which may require more reg entries
> > to describe?
>
> Nope. In age of eMMC I wouldn't even expect any new OneNAND controller
> to be handled by these bindings.

Ok.

>
> > > + - interrupt-parent, interrupts Onenand memory interrupts
> >
> > As it's a common property, I don't think interrupt-parent needs to
> > be described.
> >
> > Is there more than one interrupt? What's it called on the manual?
>
> There is one interrupt per OneNAND chip, so this is what should be
> represented in the binding.

Ah. That should be mentioned explicitly, with the required ordering.

>
> > > +
> > > +Required properties for others:
> > > +
> > > + - reg: the offset and length of the control registers. First region describes
> > > + control registers, second OneNAND interface.
> >
> > Why does the s5pc110 OneNAND binding take its registers in the opposite
> > order to the rest of these? Can we not fix up the driver first to make
> > it consistent? Then we only need to describe this once and it's going to
> > be far less of a headache to support.
>
> Both areas in both "major" hardware variants have completely different
> meaning:
> - in case of SoCs earlier or equal to S5PC100, first entry represents
> controller registers that are used to control various operating
> parameters, while second entry is a command/data interface, which is
> used to trigger read/write/etc. operations in the controller and
> push/pull data through it. In this variant there is no direct mapping
> of OneNAND chip in CPU address space. This variant supports only
> one memory chip per controller instance,
> - in case of S5PC110, each OneNAND chip is directly mapped into CPU
> address space. In addition there is a block of control registers that
> are used to configure the interface and control internal DMA engine
> (which is not present in <= S5PC100 variants). This variant supports
> multiple memory chips per controller instance (2 in case of S5PV210,
> but current driver handles only one AFAIK).

Ok. Even with that, I think the suggestion to reorder the reg entries
above still makes this more consistent, with the controller registers
first, then an entry per-chip (which is either the chip itself or the
related command/data interface depending on the particular controller).

>
> > > +
> > > +Clocks:
> > > + - gate - clock which output is supplied to external OneNAND flash memory.
> >
> > How about the following (without the Clocks header):
> >
> > - clocks: clock-specifiers for the clocks named in clock-names, per the
> > common clock bindings.
> >
> > - clock-names: should contain "gate" for the clock to the OneNAND flash
> > memory.
> >
> > Is this the only clock that might be necessary on all platforms?
>
> S3C64xx SoCs have only one OneNAND gate clock per controller.
> S5PC100 SoC has one IP gate clock and one memory chip gate clock.
> S5PC110 SoC has one gate that gates both IP and all memory chips.

Good to know. Thanks for the thorough description.

Cheers,
Mark.
--
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/