linux-kernel - Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5305F898.6020401@ti.com>
Date:	Thu, 20 Feb 2014 14:44:08 +0200
From:	Ivan Khoronzhuk <ivan.khoronzhuk@...com>
To:	Mark Rutland <mark.rutland@....com>
CC:	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"santosh.shilimkar@...com" <santosh.shilimkar@...com>,
	"galak@...nel.crashing.org" <galak@...nel.crashing.org>,
	"rob@...dley.net" <rob@...dley.net>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Pawel Moll <Pawel.Moll@....com>,
	"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
	"swarren@...dotorg.org" <swarren@...dotorg.org>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	"grygorii.strashko@...com" <grygorii.strashko@...com>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"nsekhar@...com" <nsekhar@...com>
Subject: Re: [PATCH v5 2/2] memory: ti-aemif: add bindings for AEMIF driver


On 02/19/2014 08:11 PM, Mark Rutland wrote:
> On Wed, Feb 19, 2014 at 01:40:10PM +0000, Ivan Khoronzhuk wrote:
>> Add bindings for TI Async External Memory Interface (AEMIF) controller.
>>
>> The Async External Memory Interface (EMIF16/AEMIF) controller is intended to
>> provide a glue-less interface to a variety of asynchronous memory devices like
>> ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories
>> can be accessed via 4 chip selects with 64M byte access per chip select.
>>
>> We are not encoding CS number in reg property, it's memory partition number.
>> The CS number is encoded for Davinci NAND node using standalone property
>> "ti,davinci-chipselect" and we need to provide two memory ranges to it,
>> as result we can't encode CS number in "reg" for AEMIF child devices
>> (NAND/NOR/etc), as it will break bindings compatibility.
>>
>> In this patch, NAND node is used just as an example of child node.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@...com>
>> ---
>>   .../bindings/memory-controllers/ti-aemif.txt       | 210 +++++++++++++++++++++
>>   1 file changed, 210 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/memory-controllers/ti-aemif.txt
> [...]
>
>> +- ranges:		Contains memory regions. There are two types of
>> +			ranges/partitions:
>> +			- CS-specific partition/range. If continuous, must be
>> +			set up to reflect the memory layout for 4 chipselects,
>> +			if not then additional range/partition can be added and
>> +			child device can select the proper one.
>> +			- control partition which is common for all CS
>> +			interfaces.
> This really doesn't sound anything like the typical meaning of ranges on
> a bus.
>
> Why isn't this information encoded in reg properties on the child nodes?
>
> [...]

Note that we do not introduce NAND device bindings here.
The Davinci NAND bindings was introduced and accepted more then one year ago.
And the CS number is encoded for Davinci NAND node using standalone property
"ti,davinci-chipselect" and we need to provide two memory ranges to it,
as result we can't encode CS number in "reg" for AEMIF child devices (NAND/NOR/etc),
as it will break bindings compatibility.

In this document, NAND node is used just as an example of child node.

>> +- ti,cs-ss:		enable/disable select strobe mode
>> +				In select strobe mode chip select behaves as
>> +				the strobe and is active only during the strobe
>> +				period. If present then enable.
>> +
>> +- ti,cs-ew:		enable/disable extended wait mode
>> +				if set, the controller monitors the EMIFWAIT pin
>> +				mapped to that chip select to determine if the
>> +				device wants to extend the strobe period. If
>> +				present then enable.
> When would you have these two in the DT and when wouldn't you? Why can't
> the driver decide?

These are properties of cs node, not the driver itself.
The driver cannot know what child device you are going to attach to cs node
, as result it cannot decide what settings you are using for particular 
cs node.

> These names are also opaque. We can clearly do better.

I propose the following names?:

ti,cs-ss  --> ti,cs-select-strobe-mode
ti,cs-ew --> ti,cs-extended-waite-mode

Are you OK with it?

>> +
>> +- ti,cs-ta:		minimum turn around time, ns
>> +				Time between the end of one asynchronous memory
>> +				access and the start of another asynchronous
>> +				memory access. This delay is not incurred
>> +				between a read followed by read or a write
>> +				followed by a write to same chip select.
> The name is opaque. How about ti,min-turnaround-time-ns ?

I like without -ns suffix and with cs- prefix:
ti,cs-ta --> ti,cs-min-turnaround-time

Is it OK?

>> +
>> +- ti,cs-rsetup:		read setup width, ns
>> +				Time between the beginning of a memory cycle
>> +				and the activation of read strobe.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-rstobe:		read strobe width, ns
>> +				Time between the activation and deactivation of
>> +				the read strobe.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-rhold:		read hold width, ns
>> +				Time between the deactivation of the read
>> +				strobe and the end of the cycle (which may be
>> +				either an address change or the deactivation of
>> +				the chip select signal.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-wsetup:		write setup width, ns
>> +				Time between the beginning of a memory cycle
>> +				and the activation of write strobe.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-wstrobe:	write strobe width, ns
>> +				Time between the activation and deactivation of
>> +				the write strobe.
>> +				Minimum value is 1 (0 treated as 1).
>> +
>> +- ti,cs-whold:		write hold width, ns
>> +				Time between the deactivation of the write
>> +				strobe and the end of the cycle (which may be
>> +				either an address change or the deactivation of
>> +				the chip select signal.
>> +				Minimum value is 1 (0 treated as 1).
> Likewise I think these can be given more descriptive names.

Ok. How about:

ti,cs-rsetup --> ti,cs-read-setup-time
ti,cs-rstobe --> ti,cs-read-strobe-time
ti,cs-rhold  --> ti,cs-read-hold-time
ti,cs-wsetup --> ti,cs-write-setup-time
ti,cs-wstrobe --> ti,cs-write-strobe-time
ti,cs-whold --> ti,cs-write-hold-time

Thanks

-- 
Regards,
Ivan Khoronzhuk

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/