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

[Wolfgang Denk]

Dear Timur Tabi,

In message <AANLkTinbCYBbwksm5XX5psWlYqpB_dZ6-DaOuhuKkM-S at mail.gmail.com> you wrote:
...
> >> +/* ranges for parameters:
> >> + * =A0wr_data_delay =3D 0-6
> >> + * =A0clk adjust =3D 0-8
> >> + * =A0cpo 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 =3D 0-6
>  *  clk adjust =3D 0-8
>  *  cpo 2-0x1E (30)
>  */

Yes,indeed. Please see the Coding Style document in case of further
questions.

> > Please use TABs for vertical alignment.
> 
> I thought I did.

You did not.

> >> +     /* 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.

Please add such a comment, then.

> >> +     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.

fsl_ddr_sdram() does NOT the same as get_ram_size(). It does not
perform any actual testing, and will not detect errors in the SPD
information or in the code processing it.

...
> >> +     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.

I should have been more specific here. Hiere, at the first repetition
of this code, I only meant you should drop the ``puts("\n");'' line.

> 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.

Thanks for pointing this out. so we have even more occurrences of this
block of code - time has come to factor this out into a common
function that is used for this board and for p2020ds as well.

> > 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.

Actually it is not that many arguments.

In any case, there are so many occurrences of this block of code that
it really should be turned into a function.

> >> +#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.

Please document it.

> > 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?

I complain about things when I see them.

Do you really expect me to review all patches submitted on this list
in scrupulous detail? I skim through most of them. I read many of
them more carefully. I try to review as many patches as time permits,
but I never claimed 100% coverage.

Maybe this slept through in the past.

Now it got caught.


It would be nice if you also fixed all the other dozen places where
such code has crept in.

> >> +#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.

You mean neither of you knows if this is correct or wrong ???

> >> +++ 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.

No, it's not. It is largely the same as mpc8610hpcd_diu.c; common
video driver code belongs into drivers/video/


> > 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.

It's only about 200 LOC, and large parts look pretty similar. Please
do not duplicate all this, instead factor our the common parts.

> >> +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.

You know it know.

> >> +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?

It's duplicating existing code ("bmp" command).

> >> +#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.

I'm not. I'm just reviewing a patch.

It would be great if you cleaned up the other places as well.


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 biggest difference between time and space is that you can't reuse
time.                                                 - Merrick Furst