From: Boris Brezillon <boris.brezillon@free-electrons.com>
To: Rob Herring <robh@kernel.org>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>,
Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>,
Alexandre Belloni <alexandre.belloni@free-electrons.com>,
Pawel Moll <pawel.moll@arm.com>,
Mark Rutland <mark.rutland@arm.com>,
Ian Campbell <ijc+devicetree@hellion.org.uk>,
Kumar Gala <galak@codeaurora.org>,
devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org, Arnd Bergmann <arnd@arndb.de>,
Jean-Jacques Hiblot <jjhiblot@traphandler.com>
Subject: Re: [PATCH v7 2/2] memory: atmel-ebi: add DT bindings documentation
Date: Tue, 3 May 2016 18:51:53 +0200 [thread overview]
Message-ID: <20160503185153.5cfddc32@bbrezillon> (raw)
In-Reply-To: <20160503164019.GA1604@rob-hp-laptop>
Hi Rob,
On Tue, 3 May 2016 11:40:19 -0500
Rob Herring <robh@kernel.org> wrote:
> On Thu, Apr 28, 2016 at 02:03:27PM +0200, Boris Brezillon wrote:
> > The EBI (External Bus Interface) is used to access external peripherals
> > (NOR, SRAM, NAND, and other specific devices like ethernet controllers).
> > Each device is assigned a CS line and an address range and can have its
> > own configuration (timings, access mode, bus width, ...).
> > This driver provides a generic DT binding to configure a device according
> > to its requirements.
> > For specific device controllers (like the NAND one) the SMC timings
> > should be configured by the controller driver through the matrix and smc
> > syscon regmaps.
> >
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> > .../bindings/memory-controllers/atmel,ebi.txt | 136 +++++++++++++++++++++
> > 1 file changed, 136 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> >
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > new file mode 100644
> > index 0000000..a6dca5c
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > @@ -0,0 +1,136 @@
> > +* Device tree bindings for Atmel EBI
> > +
> > +The External Bus Interface (EBI) controller is a bus where you can connect
> > +asynchronous (NAND, NOR, SRAM, ....) and synchronous memories (SDR/DDR SDRAMs).
> > +The EBI provides a glue-less interface to asynchronous memories through the SMC
> > +(Static Memory Controller).
> > +
> > +Required properties:
> > +
> > +- compatible: "atmel,at91sam9260-ebi"
> > + "atmel,at91sam9261-ebi"
> > + "atmel,at91sam9263-ebi0"
> > + "atmel,at91sam9263-ebi1"
>
> What are the differences between 0 and 1?
Because this SoC has 2 EBI busses with different capabilities.
>
> > + "atmel,at91sam9rl-ebi"
> > + "atmel,at91sam9g45-ebi"
> > + "atmel,at91sam9x5-ebi"
> > + "atmel,sama5d3-ebi"
> > +
> > +- reg: Contains offset/length value for EBI memory mapping.
> > + This property might contain several entries if the EBI
> > + memory range is not contiguous
> > +
> > +- #address-cells: Must be 2.
> > + The first cell encodes the CS.
> > + The second cell encode the offset into the CS memory
> > + range.
> > +
> > +- #size-cells: Must be set to 1.
> > +
> > +- ranges: Encodes CS to memory region association.
> > +
> > +- clocks: Clock feeding the EBI controller.
> > + See clock-bindings.txt
> > +
> > +Children device nodes are representing device connected to the EBI bus.
> > +
> > +Required device node properties:
> > +
> > +- #reg: Contains the chip-select id, the offset and the length
>
> s/#reg/reg/
Will fix that.
>
> > + of the memory region requested by the device.
> > +
> > +EBI bus configuration associated with specific chip-select will be defined in
> > +the configs subnode. This configs node will in turn contain several subnodes
> > +named config-<cs-id>, each of them containing the following properties.
>
> This is a bit unusual. Why not just part of the child device nodes?
Oh, come on! I reworked the binding because Mark complained about the
previous binding which was doing exactly what you're suggesting. Can
you please be consistent in your reviews...
>
> > +
> > +Optional config-<cs-id> node properties:
> > +
> > +- atmel,bus-width: width of the asynchronous device's data bus
> > + 8, 16 or 32.
> > + Default to 8 when undefined.
> > +
> > +- atmel,byte-access-type "write" or "select" (see Atmel datasheet).
> > + Default to "select" when undefined.
> > +
> > +- atmel,read-mode "nrd" or "ncs".
> > + Default to "ncs" when undefined.
> > +
> > +- atmel,write-mode "nwe" or "ncs".
> > + Default to "ncs" when undefined.
> > +
> > +- atmel,exnw-mode "disabled", "frozen" or "ready".
> > + Default to "disabled" when undefined.
> > +
> > +- atmel,page-mode enable page mode if present. The provided value
> > + defines the page size (supported values: 4, 8,
> > + 16 and 32).
> > +
> > +- atmel,tdf-mode: "normal" or "optimized". When set to
>
> This should be boolean.
It was a formerly defined as a boolean, and when it's done like that we
have no way to identify whether the property was forgotten or
intentionally set to normal mode. What's the problem with this approach?
>
> > + "optimized" the data float time is optimized
> > + depending on the next device being accessed
> > + (next device setup time is subtracted to the
> > + current device data float time).
> > + Default to "normal" when undefined.
> > +
> > +Mandatory timings expressed in nanoseconds (see Atmel datasheet for a full
> > +description).
>
> Required first, then optional properties please.
Okay.
>
> > +
> > +- atmel,ncs-rd-setup-ns
> > +- atmel,nrd-setup-ns
> > +- atmel,ncs-wr-setup-ns
> > +- atmel,nwe-setup-ns
> > +- atmel,ncs-rd-pulse-ns
> > +- atmel,nrd-pulse-ns
> > +- atmel,ncs-wr-pulse-ns
> > +- atmel,nwe-pulse-ns
> > +- atmel,nwe-cycle-ns
> > +- atmel,nrd-cycle-ns
> > +- atmel,tdf-ns
> > +
> > +Example:
> > +
> > + ebi: ebi@10000000 {
> > + compatible = "atmel,sama5d3-ebi";
> > + #address-cells = <2>;
> > + #size-cells = <1>;
>
> > + atmel,smc = <&hsmc>;
> > + atmel,matrix = <&matrix>;
>
> What are these?
>
> > + reg = <0x10000000 0x10000000
> > + 0x40000000 0x30000000>;
> > + ranges = <0x0 0x0 0x10000000 0x10000000
> > + 0x1 0x0 0x40000000 0x10000000
> > + 0x2 0x0 0x50000000 0x10000000
> > + 0x3 0x0 0x60000000 0x10000000>;
> > + clocks = <&mck>;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&pinctrl_ebi_addr>;
>
> Not documented.
Will document those properties.
Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
next prev parent reply other threads:[~2016-05-03 16:52 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-28 12:03 [PATCH v7 1/2] memory: add Atmel EBI (External Bus Interface) driver Boris Brezillon
2016-04-28 12:03 ` [PATCH v7 2/2] memory: atmel-ebi: add DT bindings documentation Boris Brezillon
2016-05-03 16:40 ` Rob Herring
2016-05-03 16:51 ` Boris Brezillon [this message]
2016-05-03 19:11 ` Rob Herring
2016-05-04 9:35 ` Boris Brezillon
2016-05-04 9:38 ` Boris Brezillon
2016-05-04 13:06 ` Rob Herring
2016-05-04 13:35 ` Boris Brezillon
2016-05-10 8:04 ` Boris Brezillon
2016-05-10 11:07 ` Mark Rutland
2016-05-10 12:41 ` Boris Brezillon
2016-05-10 13:08 ` Jean-Jacques Hiblot
2016-05-10 14:52 ` Rob Herring
2016-05-10 15:47 ` Boris Brezillon
2016-05-04 10:06 ` Jean-Jacques Hiblot
2016-05-04 12:43 ` Rob Herring
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160503185153.5cfddc32@bbrezillon \
--to=boris.brezillon@free-electrons.com \
--cc=alexandre.belloni@free-electrons.com \
--cc=arnd@arndb.de \
--cc=devicetree@vger.kernel.org \
--cc=galak@codeaurora.org \
--cc=ijc+devicetree@hellion.org.uk \
--cc=jjhiblot@traphandler.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=nicolas.ferre@atmel.com \
--cc=pawel.moll@arm.com \
--cc=plagnioj@jcrosoft.com \
--cc=robh@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).