From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chin Liang See Date: Tue, 1 Sep 2015 04:10:44 -0500 Subject: [U-Boot] [PATCH 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel In-Reply-To: <201509011101.07423.marex@denx.de> References: <1439963690-2523-1-git-send-email-clsee@altera.com> <1440572040.1870.8.camel@clsee-VirtualBox> <1441097646.1842.1.camel@clsee-VirtualBox> <201509011101.07423.marex@denx.de> Message-ID: <1441098644.1901.1.camel@clsee-VirtualBox> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 > wrote: > > > > >>>>>>> +CC: Simon Glass > > > > >>>>>>> > > > > >>>>>>> On Thu, Aug 20, 2015 at 12:32 AM, Marek Vasut > 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 > > > > >>>>>>>>>>>>> Cc: Dinh Nguyen > > > > >>>>>>> > > > > >>>>>>> I think there's something wrong with your git scripts, I did > > > > >>>>>>> not get this email. > > > > >>>>>>> > > > > >>>>>>>>>>>>> Cc: Pavel Machek > > > > >>>>>>>>>>>>> Cc: Marek Vasut > > > > >>>>>>>>>>>>> Cc: Wolfgang Denk > > > > >>>>>>>>>>>>> Cc: Stefan Roese > > > > >>>>>>>>>>>>> Cc: Tom Rini > > > > >>>>>>>>>>>>> --- > > > > >>>>>>>>>>>>> > > > > >>>>>>>>>>>>> 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 > > > > >>>>>>>>>>>>> #include > > > > >>>>>>>>>>>>> #include > > > > >>>>>>>>>>>>> > > > > >>>>>>>>>>>>> +#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