[U-Boot] [PATCH] powerpc: add support for the Freescale P1022DS reference board

[Timur Tabi]

On Thu, May 20, 2010 at 5:33 PM, Wolfgang Denk <wd at denx.de> wrote:

> Entries to MAKEALL and MAINTAINERS missing.

Ok.

>> +P1022DS_config:              unconfig
>> +     @$(MKCONFIG) -t $(@:_config=) P1022DS powerpc mpc85xx p1022ds freescale
>> +
>>  P1011RDB_config      \
>>  P1011RDB_NAND_config \
>>  P1011RDB_SDCARD_config \
>
> Please keep lists sorted / sort them.

Ok.

>> +void fsl_ddr_get_spd(ddr3_spd_eeprom_t *ctrl_dimms_spd, unsigned int ctrl_num)
>> +{
>> +     int ret;
>> +
>> +     /* The P1022 has only one DDR controller, and the board has only one
>> +        DIMM slot. */
>
> Incorrect multiline comment style.

Ok.

>
> ...
>> +/* ranges for parameters:
>> + *  wr_data_delay = 0-6
>> + *  clk adjust = 0-8
>> + *  cpo 2-0x1E (30)
>> + */
>
> Incorrect multiline comment style.

Sorry, what's wrong with this, specifically?  Should it look like this:

/*
 * ranges for parameters:
 *  wr_data_delay = 0-6
 *  clk adjust = 0-8
 *  cpo 2-0x1E (30)
 */

Please keep in mind that a lot of this code is taken from the existing
p2020ds support that's already in your repository, so many of your
comments are really criticisms of code that you have already accepted.

>> +     {  0, 333,    1,    5,  31,    3,  0},
>> +     {334, 400,    1,    5,  31,    3,  0},
>> +     {401, 549,    1,    5,  31,    3,  0},
>> +     {550, 680,    1,    5,  31,    5,  0},
>> +     {681, 850,    1,    5,  31,    5,  0},
>> +     {  0, 333,    2,    5,  31,    3,  0},
>> +     {334, 400,    2,    5,  31,    3,  0},
>> +     {401, 549,    2,    5,  31,    3,  0},
>> +     {550, 680,    2,    5,  31,    5,  0},
>> +     {681, 850,    2,    5,  31,    5,  0},
>
> Please use TABs for vertical alignment.

I thought I did.

>> +void fsl_ddr_board_options(memctl_options_t *popts, dimm_params_t *pdimm,
>> +                        unsigned int ctrl_num)
>> +{
> ...
>> +     /* Per AN4039, enable ZQ calibration. */
>> +     popts->zq_en = 1;
>> +
>> +
>> +     /*
>
> Drop one of these empty lines, please.

Ok.  I don't know how that got in there.

>
>> +int board_early_init_f(void)
>> +{
>> +     ccsr_gur_t *gur = (void *)CONFIG_SYS_MPC85xx_GUTS_ADDR;
>> +
>> +     /* Set pmuxcr to allow both i2c1 and i2c2 */
>> +     setbits_be32(&gur->pmuxcr, 0x1000);
>> +     in_be32(&gur->pmuxcr);
>
> What is this in_be32() needed for? Either add a comment why it's
> needed, or remove it.

Ok.  It's for serializing the I/O.  The PIXIS won't complete the read
until after the write is finished.

>> +phys_size_t initdram(int board_type)
>> +{
>> +     phys_size_t dram_size = 0;
>> +
>> +     puts("Initializing....\n");
>> +
>> +     dram_size = fsl_ddr_sdram();
>> +     dram_size = setup_ddr_tlbs(dram_size / 0x100000);
>> +     dram_size *= 0x100000;
>> +
>> +     puts("    DDR: ");
>> +     return dram_size;
>
> How about using get_ram_size() for autosizing / testing?

That's what fsl_ddr_sdram() does.  It returns the size based on SPD.

>> +     /*  Enable the TFP410 Encoder (I2C address 0x38)
>> +     */
>
> Please use a symbolic name instead of the hard-wired constant.

Ok.

>> +void pci_init_board(void)
>> +{
>> +     volatile ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
>> +     struct fsl_pci_info pci_info[3];
>> +     u32 devdisr, pordevsr, io_sel;
>> +     int first_free_busno = 0;
>> +     int num = 0;
>> +
>> +     int pcie_ep, pcie_configured;
>> +
>> +     devdisr = in_be32(&gur->devdisr);
>> +     pordevsr = in_be32(&gur->pordevsr);
>> +     io_sel = (pordevsr & MPC85xx_PORDEVSR_IO_SEL) >> 18;
>> +
>> +     debug ("   pci_init_board: devdisr=%x, io_sel=%x\n", devdisr, io_sel);
>> +
>> +     if (!(pordevsr & MPC85xx_PORDEVSR_SGMII2_DIS))
>> +             printf("    eTSEC2 is in sgmii mode.\n");
>> +     if (!(pordevsr & MPC85xx_PORDEVSR_SGMII3_DIS))
>> +             printf("    eTSEC3 is in sgmii mode.\n");
>> +
>> +     puts("\n");
>
> Drop that.

This code is identical to the code in the p2020ds.c, so I'm just
mirroring it.  The only difference is the names of the slots in the
printf.  I would prefer to keep this new code as close as possible to
our existing code.  I suspect that we'll be unifying the PCI code in
the future, and keeping it similar will make it easier.

>> +#ifdef CONFIG_PCIE1
>> +     pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_1, io_sel);
>> +
>> +     if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE)) {
>> +             SET_STD_PCIE_INFO(pci_info[num], 1);
>> +             pcie_ep = fsl_setup_hose(&pcie1_hose, pci_info[num].regs);
>> +             printf("    PCIE1 connected to Slot 1 as %s (base addr %lx)\n",
>> +                             pcie_ep ? "Endpoint" : "Root Complex",
>> +                             pci_info[num].regs);
>> +             first_free_busno = fsl_pci_init_port(&pci_info[num++],
>> +                                     &pcie1_hose, first_free_busno);
>> +     } else {
>> +             printf("    PCIE1: disabled\n");
>> +     }
>> +     puts("\n");
>
> Ditto.
>
>
> Hm... looks as if you were repeating the same code 3 times. Please make
> this a function.

The code isn't really the same.  I would need to pass a lot of
parameters to this function: the hose, the devdisr mask, the slot
name, the slot number, the bus number, and so on.

>> +#ifdef CONFIG_GET_CLK_FROM_ICS307
>
> This CONFIG_GET_CLK_FROM_ICS307 is documented?

We've been using it for years.  Now you complain?  It's the same code
in almost all of our recent boards.

>> +static unsigned long ics307_clk_freq(unsigned char cw0, unsigned char cw1,
>> +                                  unsigned char cw2)
>> +{
>> +     const unsigned long InputFrequency = CONFIG_ICS307_REFCLK_HZ;
>> +     unsigned long VDW = ((cw1 << 1) & 0x1FE) + ((cw2 >> 7) & 1);
>> +     unsigned long RDW = cw2 & 0x7F;
>> +     unsigned long OD = ics307_S_to_OD[cw0 & 0x7];
>> +     unsigned long freq;
>
> Please do not use any CamelCase or UPPER CASE identifiers.

Again, this is the same code as always.  You've accepted this code a
dozen times over.  Why are you picking on me about it now?

>> +#ifdef CONFIG_PCIE3
>> +     ft_fsl_pci_setup(blob, "pci2", &pcie3_hose);
>> +#endif
>> +#ifdef CONFIG_PCIE2
>> +     ft_fsl_pci_setup(blob, "pci0", &pcie2_hose);
>> +#endif
>> +#ifdef CONFIG_PCIE1
>> +     ft_fsl_pci_setup(blob, "pci1", &pcie1_hose);
>> +#endif
>
> Is this intentional? Looks weird to me...
>
> PCIE1 => pci1 => pcie1_hose
> PCIE2 => pci0 => pcie2_hose
> PCIE3 => pci2 => pcie3_hose
>
> Weird, weird...

I asked Kumar about it, but he didn't really have much to say, so I left it.

>> +++ b/board/freescale/p1022ds/p1022ds_diu.c
>> @@ -0,0 +1,151 @@
>
> This should probably go to drivers/video/ ?

It's p1022-specific, so I don't see why.

> Hmmm... this looks pretty much similar to
> board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c;
>
> I think you should merge these two into one driver.

And one day I will.  But not today.  It's going to take me a long time
to analyze both code and test it on both boards.  I can't solve every
problem at once.

>
>> +int p1022diu_init_show_bmp(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>> +{
>> +     unsigned int addr;
>
> ...while doing so, please clean up to use the standard bmp command
> instead.

I didn't know there was a standard bmp command.

>> +U_BOOT_CMD(
>> +     diufb, CONFIG_SYS_MAXARGS, 1, p1022diu_init_show_bmp,
>> +     "Init or Display BMP file",
>> +     "init\n    - initialize DIU\n"
>> +     "addr\n    - display bmp at address 'addr'"
>> +);
>
> This should go away.

What's wrong with it?

>> diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
>> new file mode 100644
>> index 0000000..65a1265
>> --- /dev/null
>> +++ b/include/configs/P1022DS.h
> ...
>> +#ifndef __ASSEMBLY__
>> +extern unsigned long calculate_board_sys_clk(void);
>> +extern unsigned long calculate_board_ddr_clk(void);
>> +#endif
>
> Please move to appropriate header file.

OMG, every single one of our header files does this!!!!  We've been
doing this for years!

I really do believe you're picking on me now.

>
>> +/*
>> + * Base addresses -- Note these are effective addresses where the
>> + * actual resources get mapped (not physical addresses)
>> + */
>> +#define CONFIG_SYS_CCSRBAR_DEFAULT   0xff700000      /* CCSRBAR Default */
>> +#define CONFIG_SYS_CCSRBAR           0xffe00000      /* relocated CCSRBAR */
>> +#define CONFIG_SYS_CCSRBAR_PHYS              0xfffe00000ull  /* physical addr of CCSRBAR */
>> +#define CONFIG_SYS_IMMR                      CONFIG_SYS_CCSRBAR      /* PQII uses CONFIG_SYS_IMMR */
>
> Lines too long. Please fix globally.

Ok.

>
>> +/* ECC will be enabled based on perf_mode environment variable */
>> +//#define    CONFIG_DDR_ECC
>
> No C++ comments. Please fix globally.

Oops, I forgot to delete that one.

>> +/* video */
>> +/* #define CONFIG_VIDEO */
>
> Please do not add dead code. Please fix globally.

Ok.

>
>> +#define CONFIG_BOOTDELAY 10  /* -1 disables auto-boot */
>> +#undef  CONFIG_BOOTARGS              /* the boot command will set bootargs */
>
> Do not undefine what is not defined anyway.

Sorry, I don't know how that got in there.  My eyes saw #define and not #undef.

-- 
Timur Tabi
Linux kernel developer at Freescale