[U-Boot] [PATCH 2/2 v4] powerpc/p1023rds: Disable nor flash node and enable nand flash node

[U-Boot] [PATCH 2/2 v4] powerpc/p1023rds: Disable nor flash node and enable nand flash node

[Chunhe Lan]

In the p1023rds, accessing exclusively nor flash or nand flash device by
BR0/OR0.

Default device tree nor and nand node should have the following structure:

	Example:

		nor_flash: nor at 0,0 {
			#address-cells = <1>;
			#size-cells = <1>;
			compatible = "cfi-flash";
			reg = <0x0 0x0 0x02000000>;
			bank-width = <2>;
			device-width = <1>;

			partition at 0 {
				label = "ramdisk";
				reg = <0x00000000 0x01c00000>;
			};
		}

		nand_flash: nand at 1,0 {
			#address-cells = <1>;
			#size-cells = <1>;
			compatible = "fsl,p1023-fcm-nand",
				     "fsl,elbc-fcm-nand";
			reg = <0x2 0x0 0x00040000>;
			status = "disabled";

			u-boot-nand at 0 {
				/* This location must not be altered  */
				/* 1MB for u-boot Bootloader Image */
				reg = <0x0 0x00100000>;
				read-only;
			};
		}

When booting from nor flash, the status of nor node is null that means it
is enabled and the status of nand node is disabled in the default dts file,
so do not do anything.

But, when booting from nand flash, need to do some operations:

	o the status of nor node should be disabled.
	o the status of nand node should be enabled.

Signed-off-by: Chunhe Lan <Chunhe.Lan@freescale.com>
---
 board/freescale/p1023rds/p1023rds.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/board/freescale/p1023rds/p1023rds.c b/board/freescale/p1023rds/p1023rds.c
index 8cfd199..c99ccee 100644
--- a/board/freescale/p1023rds/p1023rds.c
+++ b/board/freescale/p1023rds/p1023rds.c
@@ -158,5 +158,10 @@ void ft_board_setup(void *blob, bd_t *bd)
 	size = getenv_bootm_size();
 
 	fdt_fixup_memory(blob, (u64)base, (u64)size);
+
+#ifdef CONFIG_NAND_U_BOOT
+	do_fixup_by_path_string(fdt, "nor_flash", "status", "disabled");
+	do_fixup_by_path_string(fdt, "nand_flash", "status", "okay");
+#endif
 }
 #endif
-- 
1.5.6.5

[U-Boot] [PATCH 2/2 v4] powerpc/p1023rds: Disable nor flash node and enable nand flash node

[Wolfgang Denk]

Dear Chunhe Lan,

In message <1314602152-9114-1-git-send-email-Chunhe.Lan@freescale.com> you wrote:
> In the p1023rds, accessing exclusively nor flash or nand flash device by
> BR0/OR0.
...
> When booting from nor flash, the status of nor node is null that means it
> is enabled and the status of nand node is disabled in the default dts file,
> so do not do anything.

It would be more intuitive to the reader, when the enabled node would
use an explicit

	status = "enabled";

> +#ifdef CONFIG_NAND_U_BOOT
> +	do_fixup_by_path_string(fdt, "nor_flash", "status", "disabled");
> +	do_fixup_by_path_string(fdt, "nand_flash", "status", "okay");
> +#endif

What does ""okay" mean?  This is not documented anywhere.  Is this
supposed to mean "enabled"?  Then please write "enabled" - for certain
configurations it is definitely OK to disable the device.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
There's an old story about the person who wished his computer were as
easy to use as his telephone. That wish has come  true,  since  I  no
longer know how to use my telephone.

[U-Boot] [PATCH 2/2 v4] powerpc/p1023rds: Disable nor flash node and enable nand flash node

[Chunhe Lan]

On Mon, 29 Aug 2011 15:42:09 +0800, Wolfgang Denk <wd@denx.de> wrote:

> Dear Chunhe Lan,
>
> In message <1314602152-9114-1-git-send-email-Chunhe.Lan@freescale.com>  
> you wrote:
>> In the p1023rds, accessing exclusively nor flash or nand flash device by
>> BR0/OR0.
> ...
>> When booting from nor flash, the status of nor node is null that means  
>> it
>> is enabled and the status of nand node is disabled in the default dts  
>> file,
>> so do not do anything.
>
> It would be more intuitive to the reader, when the enabled node would
> use an explicit
>
> 	status = "enabled";
>
>> +#ifdef CONFIG_NAND_U_BOOT
>> +	do_fixup_by_path_string(fdt, "nor_flash", "status", "disabled");
>> +	do_fixup_by_path_string(fdt, "nand_flash", "status", "okay");
>> +#endif
>
> What does ""okay" mean?  This is not documented anywhere.  Is this
> supposed to mean "enabled"?  Then please write "enabled" - for certain
> configurations it is definitely OK to disable the device.

    In the kernel, whether the status of node is enabled or not, it is  
determined by
    of_device_is_available( ).

    The following content of of_device_is_available( ):

245 /**
246  *  of_device_is_available - check if a device is available for use
247  *
248  *  @device: Node to check for availability
249  *
250  *  Returns 1 if the status property is absent or set to "okay" or  
"ok",
251  *  0 otherwise
252  */
253 int of_device_is_available(const struct device_node *device)
254 {
255         const char *status;
256         int statlen;
257
258         status = of_get_property(device, "status", &statlen);
259         if (status == NULL)
260                 return 1;
261
262         if (statlen > 0) {
263                 if (!strcmp(status, "okay") || !strcmp(status, "ok"))
264                         return 1;
265         }
266
267         return 0;
268 }

          " return 1" is enabled.
          " return 0" is disabled.
          So, we do NOT use "enabled" to set status.

    Thanks.

    -Jack Lan

> Best regards,
>
> Wolfgang Denk
>

[U-Boot] [PATCH 2/2 v4] powerpc/p1023rds: Disable nor flash node and enable nand flash node

[Wolfgang Denk]

Dear Chunhe Lan,

In message <op.v0ywqdimrxq6oa@localhost.localdomain> you wrote:
> 
> > It would be more intuitive to the reader, when the enabled node would
> > use an explicit
> >
> > 	status = "enabled";
> >
> >> +#ifdef CONFIG_NAND_U_BOOT
> >> +	do_fixup_by_path_string(fdt, "nor_flash", "status", "disabled");
> >> +	do_fixup_by_path_string(fdt, "nand_flash", "status", "okay");
> >> +#endif
> >
> > What does ""okay" mean?  This is not documented anywhere.  Is this
> > supposed to mean "enabled"?  Then please write "enabled" - for certain
> > configurations it is definitely OK to disable the device.
> 
>     In the kernel, whether the status of node is enabled or not, it is  
> determined by
>     of_device_is_available( ).

well, maybe you take my hint to understand that this is not an optimal
implementation?

>           " return 1" is enabled.
>           " return 0" is disabled.
>           So, we do NOT use "enabled" to set status.

This is highly counter-intuitive and fragile.

Also, you fail to comment on my note about the strange "okay" string
in yoru code above - this makes no sense then, either.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The bad reputation UNIX has gotten is totally undeserved, laid on by
people who don't understand, who have not gotten in there  and  tried
anything."          -- Jim Joyce, owner of Jim Joyce's UNIX Bookstore

[U-Boot] [PATCH 2/2 v4] powerpc/p1023rds: Disable nor flash node and enable nand flash node

[Scott Wood]

On 08/29/2011 02:42 AM, Wolfgang Denk wrote:
> Dear Chunhe Lan,
> 
> In message <1314602152-9114-1-git-send-email-Chunhe.Lan@freescale.com> you wrote:
>> In the p1023rds, accessing exclusively nor flash or nand flash device by
>> BR0/OR0.
> ...
>> When booting from nor flash, the status of nor node is null that means it
>> is enabled and the status of nand node is disabled in the default dts file,
>> so do not do anything.
> 
> It would be more intuitive to the reader, when the enabled node would
> use an explicit
> 
> 	status = "enabled";
> 
>> +#ifdef CONFIG_NAND_U_BOOT
>> +	do_fixup_by_path_string(fdt, "nor_flash", "status", "disabled");
>> +	do_fixup_by_path_string(fdt, "nand_flash", "status", "okay");
>> +#endif
> 
> What does ""okay" mean?  This is not documented anywhere.

It is documented in ePAPR and IEEE 1275.

> Is this supposed to mean "enabled"?

Yes, or more specifically, "The device is believed to be operational."

It's generally equivalent to having no status property at all.

> Then please write "enabled" 

Please don't redefine well-established standards.

-Scott

[U-Boot] [PATCH 2/2 v4] powerpc/p1023rds: Disable nor flash node and enable nand flash node

[Wolfgang Denk]

Dear Scott Wood,

In message <4E5BBE52.7080800@freescale.com> you wrote:
>
> >> +#ifdef CONFIG_NAND_U_BOOT
> >> +	do_fixup_by_path_string(fdt, "nor_flash", "status", "disabled");
> >> +	do_fixup_by_path_string(fdt, "nand_flash", "status", "okay");
> >> +#endif
> > 
> > What does ""okay" mean?  This is not documented anywhere.
> 
> It is documented in ePAPR and IEEE 1275.

Ah, thanks for poointing out.

> > Is this supposed to mean "enabled"?
> 
> Yes, or more specifically, "The device is believed to be operational."
> 
> It's generally equivalent to having no status property at all.
> 
> > Then please write "enabled" 
> 
> Please don't redefine well-established standards.

Agreed.  Sorry, I was not aware of this definition in ePAPR.

But I guess most lreaders will - like me - fail to recognize this
relation, and the presented documentation is more confusing than
helpful.

If the recommendation is to omit the status entry in the device tree,
we should omit it in the code above, too.   Otherwise, we should set
status to okay in both cases.  Having one here and the other there is
at best confusing.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
Remember that Beethoven wrote his first symphony in C ...

[U-Boot] [PATCH 2/2 v4] powerpc/p1023rds: Disable nor flash node and enable nand flash node

[Tabi Timur-B04825]

On Mon, Aug 29, 2011 at 2:15 AM, Chunhe Lan <Chunhe.Lan@freescale.com> wrote:
> In the p1023rds, accessing exclusively nor flash or nand flash device by
> BR0/OR0.

This is not an English sentence.  I do not understand what you're saying.

-- 
Timur Tabi
Linux kernel developer at Freescale

[U-Boot] [PATCH 2/2 v4] powerpc/p1023rds: Disable nor flash node and enable nand flash node

[Chunhe Lan]

On Tue, 30 Aug 2011 04:23:11 +0800, Tabi Timur-B04825  
<B04825@freescale.com> wrote:

> On Mon, Aug 29, 2011 at 2:15 AM, Chunhe Lan <Chunhe.Lan@freescale.com>  
> wrote:
>> In the p1023rds, accessing exclusively nor flash or nand flash device by
>> BR0/OR0.
>
> This is not an English sentence.  I do not understand what you're saying.

    OK. I will rewrite it.

    Thanks.

    -Jack Lan