linux-kernel - Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND controller

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]

Date:	Tue, 6 Oct 2015 15:25:38 -0700
From:	Scott Branden <sbranden@...adcom.com>
To:	Brian Norris <computersforpeace@...il.com>,
	Anup Patel <anup.patel@...adcom.com>
CC:	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	"Ian Campbell" <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	David Woodhouse <dwmw2@...radead.org>,
	Ray Jui <rjui@...adcom.com>,
	"Florian Fainelli" <f.fainelli@...il.com>,
	Pramod Kumar <pramodku@...adcom.com>,
	Vikram Prakash <vikramp@...adcom.com>,
	Sandeep Tripathy <tripathy@...adcom.com>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
	bcm-kernel-feedback-list <bcm-kernel-feedback-list@...adcom.com>,
	Rafal Milecki <zajec5@...il.com>
Subject: Re: [PATCH 3/5] mtd: brcmnand: Optional DT flag to reset IPROC NAND
 controller

Hi Brian,

On 15-10-06 06:41 AM, Brian Norris wrote:

>>>
>>> Is there a reason not to do this reset unconditionally? I recall this came up in
>>> discussion previously, when the OpenWRT folks were trying to integrate with
>>> BCMA, where this reset was one of the few differences between the platform-
>>> device-based driver (i.e., this one) and the BCMA based driver. Might it help
>>> simplify things a bit if we just did the same thing everywhere?
>>
>> This driver is currently shared by Cygnus and NS2.
>>
>> We had similar suggestion when this patch was reviewed
>> internally in Broadcom.
>>
>> The rationale for adding optional DT flag is as follows:
>> 1. The NAND controller reset is currently required for NS2 only so
>> that it is in sane state before any NAND commands are issued. We
>> are not sure if Cygnus and all future iProc SoCs will require NAND
>> controller reset.
>
> I'm not sure this is a very strong reason. It seems fairly reasonable in
> general to reset a HW block before using it.

Efficient Boot time is a very strong reason for needing this actually. 
We use the NAND controller in the bootROM, boot1/BL1, u-boot/UEFI, and 
then Kernel stage.  By properly initializing the controller once we do 
not need to reset it 4 different times.

>
>> 2. The NAND controller reset in probe would certainly increase
>> Linux boot time so for certain iProc SoCs we might choose avoid
>> NAND controller reset to reduce boot time if possible.
>
> I recall this reason being mentioned before. I believe this only happens
> because the brcmnand driver doesn't yet handle configuring the timing
> registers, so iProc is implicitly relying on the bootloader to configure
> the NAND timings. Perhaps it's time that we fix that. I'd rather not add
> extra DT properties unless we actually need to [1]. And having proper
> timing configuration in the Linux driver will help improve speeds for
> all users (whose timings may not be configured in the bootloader).

This is the very reason we need the optional reset property.  We need to 
have timings configured by the linux driver or not.  Yes, in some cases 
we will be relying on earlier boot stages to configure some of the hardware.

>
> I actually had some preliminary work to do some timing configuration
> according to the new timing information from nand_base.c/nand_timing.c.
> Unfortunately, I didn't complete this, and I'm no longer working at
> Broadcom, so I don't exactly have access to the HW docs for all the NAND
> controller revisions, nor do I have access to as much HW for testing...
>
> Brian
>
> [1] If we really do need a device tree differentiation, perhaps it would
> be better to just differentiate the compatible string than to have
> individual boolean properties. e.g.:
>
> 	compatible = "brcm,iproc-nand-ns2", ...;
>
As described above - the option is not SoC specific.  It is system 
specific.  In some systems we may wish to reset the NAND controller in 
linux.  In some we may wish to rely on initialization that has already 
been done to speed up boot times.



Regards,
  Scott
--
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/