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

[Chin Liang See <clsee@altera.com>]

On Tue, 2015-09-01 at 11:01 +0200, marex at denx.de wrote:
> On Tuesday, September 01, 2015 at 10:54:06 AM, Chin Liang See wrote:
> > On Wed, 2015-08-26 at 01:54 -0500, Chin Liang See wrote:
> > > On Wed, 2015-08-26 at 15:14 +0900, Jaehoon Chung wrote:
> > > > On 08/26/2015 02:47 PM, Chin Liang See wrote:
> > > > > On Wed, 2015-08-26 at 14:29 +0900, Jaehoon Chung wrote:
> > > > >> Hi,
> > > > >> 
> > > > >> On 08/25/2015 12:08 PM, Chin Liang See wrote:
> > > > >>> On Tue, 2015-08-25 at 11:36 +0900, Jaehoon Chung wrote:
> > > > >>>> Hi,
> > > > >>>> 
> > > > >>>> On 08/25/2015 12:04 AM, Chin Liang See wrote:
> > > > >>>>> On Fri, 2015-08-21 at 14:52 -0600, Simon Glass wrote:
> > > > >>>>>> Hi,
> > > > >>>>>> 
> > > > >>>>>> On 20 August 2015 at 15:55, Dinh Nguyen <dinh.linux@gmail.com> 
> wrote:
> > > > >>>>>>> +CC: Simon Glass
> > > > >>>>>>> 
> > > > >>>>>>> On Thu, Aug 20, 2015 at 12:32 AM, Marek Vasut <marex@denx.de> 
> wrote:
> > > > >>>>>>>> On Thursday, August 20, 2015 at 07:28:02 AM, Chin Liang See 
> wrote:
> > > > >>>>>>>>> Hi,
> > > > >>>>>>>>> 
> > > > >>>>>>>>> On Wed, 2015-08-19 at 14:36 +0000, marex at denx.de wrote:
> > > > >>>>>>>>>> On Wednesday, August 19, 2015 at 10:21:17 AM, Chin Liang See 
> wrote:
> > > > >>>>>>>>>>> Hi,
> > > > >>>>>>>>>> 
> > > > >>>>>>>>>> Hi again,
> > > > >>>>>>>>>> 
> > > > >>>>>>>>>>> On Wed, 2015-08-19 at 02:41 +0000, marex at denx.de wrote:
> > > > >>>>>>>>>>>> On Wednesday, August 19, 2015 at 07:54:50 AM, 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>
> > > > >>>>>>> 
> > > > >>>>>>> I think there's something wrong with your git scripts, I did
> > > > >>>>>>> not get this email.
> > > > >>>>>>> 
> > > > >>>>>>>>>>>>> Cc: Pavel Machek <pavel@denx.de>
> > > > >>>>>>>>>>>>> Cc: Marek Vasut <marex@denx.de>
> > > > >>>>>>>>>>>>> Cc: Wolfgang Denk <wd@denx.de>
> > > > >>>>>>>>>>>>> Cc: Stefan Roese <sr@denx.de>
> > > > >>>>>>>>>>>>> Cc: Tom Rini <trini@konsulko.com>
> > > > >>>>>>>>>>>>> ---
> > > > >>>>>>>>>>>>> 
> > > > >>>>>>>>>>>>>  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
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +/*
> > > > >>>>>>>>>>>>> + * 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-pro
> > > > >>>>>>>>>>>>> blem/18441 0529 + * 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;
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +     /* 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;
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +     /* Simple solution if all combinations pass */
> > > > >>>>>>>>>>>>> +     if(sum == (CAL_ROWS * CAL_COLS)) {
> > > > >>>>>>>>>>>>> +             *cal_row = (CAL_ROWS - 1) / 2;
> > > > >>>>>>>>>>>>> +             *cal_col = (CAL_COLS - 1) / 2;
> > > > >>>>>>>>>>>>> +             return 0;
> > > > >>>>>>>>>>>>> +     }
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +     /*
> > > > >>>>>>>>>>>>> +      * 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--){ +                     /* Add a new rectangle
> > > > >>>>>>>>>>>>> candidate */ +                    
> > > > >>>>>>>>>>>>> rect_cands[cr_rect_cand].height = height; +             
> > > > >>>>>>>>>>>>>        rect_cands[cr_rect_cand].width = width; +        
> > > > >>>>>>>>>>>>>             rect_cands[cr_rect_cand].area = height *
> > > > >>>>>>>>>>>>> width; +                     cr_rect_cand++;
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +                     /* First candidate it always in the
> > > > >>>>>>>>>>>>> right
> > > > >>>>>>>> 
> > > > >>>>>>>> position */
> > > > >>>>>>>> 
> > > > >>>>>>>>>>>>> +                     if(cr_rect_cand == 1)
> > > > >>>>>>>>>>>>> +                             continue;
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +                     /*
> > > > >>>>>>>>>>>>> +                      * Put the candidate in right
> > > > >>>>>>>>>>>>> location to
> > > > >>>>>>>> 
> > > > >>>>>>>> maintain
> > > > >>>>>>>> 
> > > > >>>>>>>>>>>>> +                      * descending order
> > > > >>>>>>>>>>>>> +                      */
> > > > >>>>>>>>>>>>> +                     for(k = cr_rect_cand - 1; k > 1;
> > > > >>>>>>>>>>>>> k--){ +                            
> > > > >>>>>>>>>>>>> if(rect_cands[k-1].area <
> > > > >>>>>>>> 
> > > > >>>>>>>> rect_cands[k].area){
> > > > >>>>>>>> 
> > > > >>>>>>>>>>>>> +                                     rect_cand_t tmp =
> > > > >>>>>>>> 
> > > > >>>>>>>> rect_cands[k-1];
> > > > >>>>>>>> 
> > > > >>>>>>>>>>>>> +                                     rect_cands[k-1] =
> > > > >>>>>>>>>>>>> rect_cands[k]; +                                    
> > > > >>>>>>>>>>>>> rect_cands[k] = tmp; +                             }
> > > > >>>>>>>>>>>>> else
> > > > >>>>>>>>>>>>> +                                     break;
> > > > >>>>>>>>>>>>> +                     }
> > > > >>>>>>>>>>>>> +             }
> > > > >>>>>>>>>>>>> +     }
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +     /* 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; +                                  
> > > > >>>>>>>>>>>>>           }
> > > > >>>>>>>>>>>>> +                                     }
> > > > >>>>>>>>>>>>> +                             }
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +                             /*
> > > > >>>>>>>>>>>>> +                              * Return 'middle' of
> > > > >>>>>>>>>>>>> rectangle in case
> > > > >>>>>>>> 
> > > > >>>>>>>> of
> > > > >>>>>>>> 
> > > > >>>>>>>>>>>>> +                              * success
> > > > >>>>>>>>>>>>> +                              */
> > > > >>>>>>>>>>>>> +                             if (ok) {
> > > > >>>>>>>>>>>>> +                                     if(rect_width > 1)
> > > > >>>>>>>>>>>>> +                                            
> > > > >>>>>>>>>>>>> rect_width--; +
> > > > >>>>>>>>>>>>> +                                     if(rect_height > 1)
> > > > >>>>>>>>>>>>> +                                            
> > > > >>>>>>>>>>>>> rect_height--; +
> > > > >>>>>>>>>>>>> +                                     *cal_row =
> > > > >>>>>>>>>>>>> start_row + +                                           
> > > > >>>>>>>>>>>>>     (rect_height / 2); +                                
> > > > >>>>>>>>>>>>>     *cal_col = start_col +
> > > > >>>>>>>> 
> > > > >>>>>>>> (rect_width / 2);
> > > > >>>>>>>> 
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +                                     return 0;
> > > > >>>>>>>>>>>>> +                             }
> > > > >>>>>>>>>>>>> +                     }
> > > > >>>>>>>>>>>>> +             }
> > > > >>>>>>>>>>>>> +     }
> > > > >>>>>>>>>>>>> +
> > > > >>>>>>>>>>>>> +     /* We could not fit any rectangle - return failure
> > > > >>>>>>>>>>>>> */ +     return 1;
> > > > >>>>>>>>>>>> 
> > > > >>>>>>>>>>>> Oh btw. please make sure to use errno.h and standard
> > > > >>>>>>>>>>>> return codes ; -EINVAL in this case I believe.
> > > > >>>>>>>>>>> 
> > > > >>>>>>>>>>> Good suggestion, will fix in v2.
> > > > >>>>>>>>>>> Thanks
> > > > >>>>>>>>>> 
> > > > >>>>>>>>>> Thanks. I was curious, is this really socfpga specific or
> > > > >>>>>>>>>> can this calibration be used on dwmmc in general -- thus on
> > > > >>>>>>>>>> exynos and rockchip systems -- as well? If it's generic to
> > > > >>>>>>>>>> dwmmc, this should be moved into the dwmmc core code. Also,
> > > > >>>>>>>>>> I am CCing Simon, he's been plumbing in the DWMMC recently.
> > > > >>>>>>> 
> > > > >>>>>>> I didn't see you explicitly add Simon to the thread. Doing it
> > > > >>>>>>> here..
> > > > >>>>>> 
> > > > >>>>>> One option is to put the function in the common dw_mmc.c file
> > > > >>>>>> but have it be called explicitly by drivers that want it (e.g.
> > > > >>>>>> for now, just socfpga). The code you have written does look
> > > > >>>>>> generic and perhaps should be used on other platforms,
> > > > >>>>>> 
> > > > >>>>>>>>> I am not sure. But definitely we can make the calibration
> > > > >>>>>>>>> common enough if they required the calibration too. Let wait
> > > > >>>>>>>>> feedback from Simon.
> > > > >>>>>>> 
> > > > >>>>>>> I know that the linux driver for the DW MMC  for SoCFGPA,
> > > > >>>>>>> Exynos and Rockchip are using the drvsel and smpsel
> > > > >>>>>>> parameters. But I also think these parameters can
> > > > >>>>>>> be different for different types of SD card. Are you sure this
> > > > >>>>>>> function will cover those
> > > > >>>>>>> cases?
> > > > >>>>>> 
> > > > >>>>>> Not at all, but perhaps that is something we will find out in
> > > > >>>>>> the future.
> > > > >>>>> 
> > > > >>>>> Thanks Simon.
> > > > >>>>> 
> > > > >>>>> I further checked today and noticed Exynos is setting up the
> > > > >>>>> clksel (sample clock and drive clock). But from exynos_dw_mmc.c,
> > > > >>>>> the value is coming from device tree. So I am notifying Jaehoon
> > > > >>>>> wonder the calibration might applicable for him.
> > > > >> 
> > > > >> I think this patch should not be common code at all.
> > > > >> Of course, it seems that find_calibration_point function can be used
> > > > >> at exynos. But it's too complex and not readable.
> > > > >> 
> > > > >> And smpclk and drvsel are also not common.
> > > > >> Depending on SoC, finding approach should be differed.
> > > > >> I don't know how smpclk and drvsel controlled, since i don't read
> > > > >> TRM of other SoC.
> > > > >> 
> > > > >> Does it need to find the best clksmpl and drvsel value in
> > > > >> bootloader?
> > > > > 
> > > > > Yah, its little complex as we try to find the best value for 2
> > > > > parameters together. Its also due to the nature that we rely on full
> > > > > complete transaction cycle to determine pass or fail (drvsel for out
> > > > > while clksmp for in).
> > > > 
> > > > In my experiment, spmlclk value is more important than drvsel.
> > > > Anyway, CAL_ROWS and CAL_COLS are candiates, right?
> > > 
> > > Yup, they are representing the value permutation for smplclk and drvsel.
> > > 
> > > > If it can be passed by parameter from each SoC dwmmc controller, that
> > > > function should be located into dwmmc.c. Because drvsel and smplclk
> > > > bits should be difference at each SoC specific register or Generic
> > > > dwmmc(UHS_REG_EXT regster).
> > > > 
> > > > Do you have the plan that change the more generic code than now?
> > > 
> > > Actually I look back and different SoC have different register location
> > > for drvsel and smplclk. They are not located within dwmmc controller
> > > registers. As of now, they are configure within the callback function of
> > > clksel, I believe that should good.
> > 
> > For this case, I presume the change would applicable for socfpga only
> > instead common dw_mmc. If everyone agreed, I shall send in new revision
> > of the patch.
> 
> I'm not very convinced that this should be outside of the dw_mmc core.

Would want to hear more from Jaehoon as Exynos and SOCFPGA are the one
setting up these values.

Thanks
Chin Liang