Message-ID: <20160427173720.013f46aa@bbrezillon>
Date:	Wed, 27 Apr 2016 17:37:20 +0200
From:	Boris Brezillon <boris.brezillon@...e-electrons.com>
To:	Mark Rutland <mark.rutland@....com>
Cc:	Nicolas Ferre <nicolas.ferre@...el.com>,
	Jean-Christophe Plagniol-Villard <plagnioj@...osoft.com>,
	Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, devicetree@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
	Arnd Bergmann <arnd@...db.de>,
	Jean-Jacques Hiblot <jjhiblot@...phandler.com>
Subject: Re: [PATCH v6 2/2] memory: atmel-ebi: add DT bindings documentation

Hi Mark,

On Wed, 27 Apr 2016 16:07:38 +0100
Mark Rutland <mark.rutland@....com> wrote:

> On Wed, Apr 27, 2016 at 04:35:54PM +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@...e-electrons.com>
> > ---
> >  .../bindings/memory-controllers/atmel,ebi.txt      | 146 +++++++++++++++++++++
> >  1 file changed, 146 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..0f332d2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-controllers/atmel,ebi.txt
> > @@ -0,0 +1,146 @@
> > +* 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 though the SMC
> > +(Static Memory Controller).
> > +Synchronous memories (and some asynchronous memories like NANDs) can be
> > +attached to specialized controllers which are responsible for configuring the
> > +bus appropriately according to the connected device.
> > +In the other hand, the bus interface can be automated for simple asynchronous
> > +devices.
> > +
> > +Required properties:
> > +
> > +- compatible:		"atmel,at91sam9260-ebi"
> > +			"atmel,at91sam9261-ebi"
> > +			"atmel,at91sam9263-ebi0"
> > +			"atmel,at91sam9263-ebi1"
> > +			"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
> > +
> > +Child chip-select (cs) nodes contain the memory devices nodes connected to
> > +such as NOR (e.g. cfi-flash) and NAND.
> > +There might be board specific devices like FPGAs.
> > +You'll define you device requirements in these child nodes.
> > +
> > +Required child cs node properties:
> > +
> > +- #address-cells:	Must be 2.
> > +
> > +- #size-cells:		Must be 1.
> > +
> > +- ranges:		Empty property indicating that child nodes can inherit
> > +			memory layout.
> > +
> > +Optional child cs node properties:
> > +- atmel,bus-width:		width of the asynchronous device's data bus
> > +				8, 16 or 32.
> > +				8 if not present.
> > +
> > +- atmel,byte-access-type	"write" or "select" (see Atmel datasheet).
> > +				"select" if not present.
> > +
> > +- atmel,read-mode		"nrd" or "ncs".
> > +				"ncs" is not present.
> > +
> > +- atmel,write-mode		"nwe" or "ncs".
> > +				"ncs" is not present.
> > +
> > +- atmel,exnw-mode		"disabled", "frozen" or "ready".
> > +				"disabled" if not present.
> > +
> > +- atmel,page-mode		enable page mode if present. The provided value
> > +				defines the page size (supported values: 4, 8,
> > +				16 and 32).
> > +
> > +Optional device timings expressed in nanoseconds (if the property is not
> > +present 0 is assumed):
> > +
> > +- 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  
> 
> I assume that these are defined in a datasheet. It may be worth noting
> "see datasheet" above this collection of properties.

Yep.

> 
> > +
> > +- atmel,tdf-mode:		"normal" or "optimized". If set to "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).
> > +
> > +
> > +
> > +Example:
> > +
> > +	ebi: ebi@...00000 {
> > +		compatible = "atmel,sama5d3-ebi", "simple-bus";  
> 
> I don't believe that "simple-bus" should be here.
> 
> > +		#address-cells = <2>;
> > +		#size-cells = <1>;
> > +		atmel,smc = <&hsmc>;
> > +		atmel,matrix = <&matrix>;
> > +		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>;
> > +
> > +		cs@0 {  
> 
> I suspect recent DTC will warn here, as the unit-address should match
> the reg for the node (and no reg is defined). Here the reg would have to
> be two cells, which means we have to make up an address, no?
> 
> We can either give the node a name without a unit-address (e.g. cs_0),
> or define some reg format.

I think I'll go for the cs_X or csX solution.

> 
> > +			#address-cells = <2>;
> > +			#size-cells = <1>;
> > +			ranges;
> > +			atmel,generic-dev;  
> 
> The cover letter mentioned this should go.

Yes, it's a leftover from the previous version. I'll remove it.

> 
> > +			atmel,read-mode = "nrd";
> > +			atmel,write-mode = "nwe";
> > +			atmel,bus-width = <16>;
> > +			atmel,ncs-rd-setup-ns = <0>;
> > +			atmel,ncs-wr-setup-ns = <0>;
> > +			atmel,nwe-setup-ns = <8>;
> > +			atmel,nrd-setup-ns = <16>;
> > +			atmel,ncs-rd-pulse-ns = <84>;
> > +			atmel,ncs-wr-pulse-ns = <84>;
> > +			atmel,nrd-pulse-ns = <76>;
> > +			atmel,nwe-pulse-ns = <76>;
> > +			atmel,nrd-cycle-ns = <107>;
> > +			atmel,nwe-cycle-ns = <84>;
> > +			atmel,tdf-ns = <16>;
> > +
> > +			nor: flash@0,0 {
> > +				compatible = "cfi-flash";
> > +				#address-cells = <1>;
> > +				#size-cells = <1>;
> > +				reg = <0x0 0x0 0x1000000>;  
> 
> It feels odd that in the node for chipselect N, sub-devices have to
> encode the chipselect number in their reg, when it's obvious from their
> container. It may make more sense for the cs node to have a non-empty
> reg (or somehow to make that translation/truncation implicit).

Well, it's using the ranges property to automate reg => struct resource
conversion, and the CS number is part of this conversion. Do you see a
simpler solution to do that.

Also, I'd like to mention that I'm not really happy with the current
DT representation: ideally, I'd like to have devices connected to the
EBI bus as direct sub-nodes of the ebi node, but that's not possible,
because we have to isolate EBI bus config (timings and other bus
related configs) from device description (which is driver specific).

I considered splitting the subdevices and configs descriptions in
two different subnodes, but decided to go for the representation
already used by other memory controllers (see TI AEMIF binding).

Any suggestions?

Regards,

Boris
-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Powered by blists - more mailing lists