On 06/10/15 15:25, Scott Branden wrote:

Hi Brian,

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

quoted

quoted

quoted

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.

This could be used as a reverse argument, issuing a reset will increase
the boot time.

quoted

quoted

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.

Then instead of adding a "reset flag" to Device Tree, another approach
could be to put the desired or currently configured exhaustive list of
NAND timings in Device Tree, and based on that you could have this:

- the NAND controller driver finds that these timings match the current
configuration, you are good to go

- the NAND controller drivers finds a difference in how current timings
are configured vs. desired timings, and issues a controller reset, prior
to applying new timing configuration

- no timings are configured, reset the controller and use existing
auto-detection capabilities like ONFI modes

Typically you would put the desired timings instead of the currently
configured timings though..

quoted

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.

It seems to me like having this property is fine as long as you are
describing that the controller *needs* a reset to operate properly, it
does not strike me as a particularly well suited property if its side
effect and main usage is to keep or wipe-out existing NAND timings.
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html