[U-Boot] [PATCH 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel

Chin Liang See
2015-08-19T08:30:17Z

Him

On Wed, 2015-08-19 at 09:34 +0200, marex at denx.de wrote:
> On Wednesday, August 19, 2015 at 09:26:55 AM, Pavel Machek wrote:
> > On Wed 2015-08-19 00:54:50, Chin Liang See wrote:
> > > Enable SDMMC calibration to determine the best setting for
> > > drvsel and smpsel. It will be triggered whenever there is
> > > a change of card frequency and bus width. This is to ensure
> > > reliable transmission between the controller and the card.
> > >
> > > Signed-off-by: Chin Liang See <clsee@altera.com>
> > > Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > Cc: Pavel Machek <pavel@denx.de>
> > > Cc: Marek Vasut <marex@denx.de>
> > > Cc: Wolfgang Denk <wd@denx.de>
>
> I guess there's no need to CC Wolfgang and Tom on SoCFPGA-specific stuff.

Sure, I was referring to the previous patch for the mailing list :)

>
> > > Cc: Stefan Roese <sr@denx.de>
> > > Cc: Tom Rini <trini@konsulko.com>
> > > ---
>
> Hi!
>
> > > drivers/mmc/socfpga_dw_mmc.c | 187
> > > ++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 180
> > > insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c
> > > index eb69aed..15e537c 100644
> > > --- a/drivers/mmc/socfpga_dw_mmc.c
> > > +++ b/drivers/mmc/socfpga_dw_mmc.c
> > > @@ -11,25 +11,140 @@
> > >
> > > #include <asm/arch/dwmmc.h>
> > > #include <asm/arch/clock_manager.h>
> > > #include <asm/arch/system_manager.h>
> > >
> > > +#include "mmc_private.h"
> > >
> > > static const struct socfpga_clock_manager *clock_manager_base =
> > >
> > > (void *)SOCFPGA_CLKMGR_ADDRESS;
> > >
> > > static const struct socfpga_system_manager *system_manager_base =
> > >
> > > (void *)SOCFPGA_SYSMGR_ADDRESS;
> > >
> > > -static void socfpga_dwmci_clksel(struct dwmci_host *host)
> > > +#define CAL_ROWS 7
> > > +#define CAL_COLS 8
>
> What is this ugliness needed for ?

Actually this is used to create the rectangle based on the result.
We are doing the shmoo test for the parameter smpsel and drvsel.

>
> btw. Please rebase on top of u-boot-socfpga/master .

Sure, just saw new changes has been added for last few hours.

>
> > > +/*
> > > + * This function determines the largest rectangle filled with 1's and
> > > returns + * the middle. This functon can be optimized, for example using
> > > the algorithm + * from
> >
> > http://www.drdobbs.com/database/the-maximal-rectangle-problem/184410529
> >
> > Actually, I'd like to see optimized version. It has potential to be
> > easier to read...
> >
> > > + * It currently takes less than 1ms, while creating the input data takes
> > > ~5ms + * so there is not a real need to optimize it.
> > > + */
> > > +int find_calibration_point(unsigned char
> > > cal_results[CAL_ROWS][CAL_COLS], +unsigned int sum, unsigned int *
> > > cal_row, unsigned int * cal_col)
> > >
> > > {
> > >
> > > - unsigned int drvsel;
> > > - unsigned int smplsel;
> > > + /* Structure containing a rectangle candidate */
> > > + typedef struct
> > > + {
> > > + unsigned char height;
> > > + unsigned char width;
> > > + unsigned short area;
> > > + } rect_cand_t;
>
> Also get rid of this typedef monster.

Make sense as we are using here only.

>
> > > + /* Array with the rectangle candidates */
> > > + rect_cand_t rect_cands[CAL_ROWS * CAL_COLS];
> > > + unsigned char cr_rect_cand = 0;
> > > + unsigned char height, width, k;
> > > +
> > > + /* No solution if all combinations fail */
> > > + if(sum == 0)
> > > + return 1;
> >
> > Codingstyle: "if ("
> >
> > > + /*
> > > + * Create list of all possible sub-rectangles, in descending area
> > > + * order
> > > + */
> > > + for(height = CAL_ROWS; height >= 1; height--) {
> > > + for(width = CAL_COLS; width >= 1 ; width--){
> >
> > No space before ;.
> >
> > > + /* Try to fit the rectangle candidates, in descending area order */
> > > + for(k = 0; k < CAL_ROWS * CAL_COLS; k++) {
> > > + unsigned char start_row, start_col;
> > > + unsigned rect_width = rect_cands[k].width;
> > > + unsigned rect_height = rect_cands[k].height;
> > > +
> > > + /* Find the row and column where the candidate fits */
> > > + for (start_col = 0; start_col < (CAL_COLS - rect_width + 1);
> > > + start_col++) {
> > > + for (start_row = 0; start_row < (CAL_ROWS -
> > > + rect_height + 1); start_row++) {
> > > + unsigned ok = 1;
> > > + unsigned add_col, add_row;
> > > +
> > > + /* Determine if the rectangle fits here */
> > > + for (add_col = 0; (add_col < rect_width) && ok;
> > > + add_col++) {
> > > + for (add_row = 0; add_row < rect_height;
> > > + add_row++) {
> > > + if (!cal_results
> > > + [start_row+add_row][start_col
> > > + + add_col]) {
> > > + ok = 0;
> > > + break;
> >
> > Create separate functions to make it easier to read and less indented.
>
> Right.

Let me crank my head on this :)
Thanks

Chin Liang