| Message ID | 1314602152-9114-1-git-send-email-Chunhe.Lan@freescale.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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 >
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
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
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
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.
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
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
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@0,0 { #address-cells = <1>; #size-cells = <1>; compatible = "cfi-flash"; reg = <0x0 0x0 0x02000000>; bank-width = <2>; device-width = <1>; partition@0 { label = "ramdisk"; reg = <0x00000000 0x01c00000>; }; } nand_flash: nand@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@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(-)