On Thu, 2013-08-29 at 06:10 -0500, Liu Shengzhou-B36685 wrote: > > -----Original Message----- > > From: Wood Scott-B07421 > > Sent: Wednesday, August 14, 2013 8:35 AM > > To: Liu Shengzhou-B36685 > > Cc: u-boot@lists.denx.de > > Subject: Re: [U-Boot] [PATCH] powerpc/p1010rdb-pb: add support for > > p1010rdb-pb > > board > > > > > struct law_entry law_table[] = { > > > -#ifndef CONFIG_SDCARD > > > SET_LAW(CONFIG_SYS_FLASH_BASE_PHYS, LAW_SIZE_32M, LAW_TRGT_IF_IFC), > > > SET_LAW(CONFIG_SYS_CPLD_BASE_PHYS, LAW_SIZE_128K, LAW_TRGT_IF_IFC), > > > SET_LAW(CONFIG_SYS_NAND_BASE_PHYS, LAW_SIZE_1M, LAW_TRGT_IF_IFC), > > > -#endif }; > > > > If this is applicable to the current board as well (is that P1010RDB-PA?), > > then > > it isn't related to adding PB support and thus belongs in a separate patch. > > > As P1010RDB-PA will no longer be supported officially,
Why? Don't confuse Freescale supporting the board with U-Boot supporting the board. > but we still keep previous code for P1010RDB-PA. > will add some description for P1010RDB-PA in commit message. I don't see how this answers my question. > > > +uint pin_mux; > > This is too generic for a global variable. Does it even need to be global? > > Will rename to "static uint sd_ifc_mux", need to be global for invoking in > several different functions. If it's static, it's not global. > > > +#if defined(CONFIG_P1010RDB) && defined(DEBUG) > > > void cpld_show(void) > > > { > > > struct cpld_data *cpld_data = (void *)(CONFIG_SYS_CPLD_BASE); > > > > > > - printf("CPLD: V%x.%x PCBA: V%x.0\n", > > > - in_8(&cpld_data->cpld_ver) & 0xF0, > > > - in_8(&cpld_data->cpld_ver) & 0x0F, > > > - in_8(&cpld_data->pcba_ver) & 0x0F); > > > > Why are you removing this? Where is cpld_show() called? > > > previous code for debug, actually no longer needed, will remove cpld_show(). Make such cleanup a separate patch. > > > @@ -246,6 +446,16 @@ void fdt_del_sdhc(void *blob) > > > } > > > } > > > > > > +void fdt_del_ifc(void *blob) > > > +{ > > > + int nodeoff = 0; > > > + > > > + while ((nodeoff = fdt_node_offset_by_compatible(blob, 0, > > > + "fsl,ifc")) >= 0) { > > > + fdt_del_node(blob, nodeoff); > > > + } > > > +} > > > > Is this PB-specific? If no, why is it in this patch? If not, why isn't the > > caller guarded by the PB ifdef? > > > for both PA and PB, this patch also tune for PA(though PA no longer be > supported officially). Why is it in this patch? > > > + > > > +U_BOOT_CMD( > > > + mux, 2, 0, pin_mux_cmd, > > > + "configure multiplexing pin for IFC/SDHC bus in runtime", > > > + "bus_type (e.g. mux sdhc)" > > > +); > > > > Are you sure this is a good idea? What happens to the drivers using said > > hardware at the time? Granted they should be idle when not running a > > command > > that actively uses them, but still... Usually we use hwconfig for this > > sort of thing. > > The patch supports two ways simultaneously: > 1) mux command: for temporary use case in runtime for accessing IFC and SDHC > without reboot, > this way is very useful in practice and in some test cases. > 2) hwconfig: for long-term use case. This should be a separate patch from basic board support. > > > -#define CONFIG_SYS_DDR_MODE_2_800 0x8000c000 > > > +#define CONFIG_SYS_DDR_MODE_1_800 0x00441420 > > > +#define CONFIG_SYS_DDR_MODE_2_800 0x00000000 > > > #define CONFIG_SYS_DDR_INTERVAL_800 0x0C300100 > > > -#define CONFIG_SYS_DDR_WRLVL_CONTROL_800 0x8655A608 > > > +#define CONFIG_SYS_DDR_WRLVL_CONTROL_800 0x8675f608 > > > > Shouldn't this be ifdeffed by the board revision? > > No, this is for both PA and PB. old parameters were not the optimal, > will add some description for P1010RDB-PA in commit message. Separate patch. -Scott _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot