[U-Boot] [PATCH 1/2] arm: socfpga: mmc: Enable calibration for drvsel and smpsel
Dinh Nguyen
dinguyen at opensource.altera.com
Tue Sep 1 16:53:44 CEST 2015
On 09/01/2015 05:12 AM, Jaehoon Chung wrote:
> On 09/01/2015 06:10 PM, Chin Liang See wrote:
>> 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 at gmail.com>
>>> wrote:
>>>>>>>>>>>>> +CC: Simon Glass
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Aug 20, 2015 at 12:32 AM, Marek Vasut <marex at 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 at altera.com>
>>>>>>>>>>>>>>>>>>> Cc: Dinh Nguyen <dinguyen at opensource.altera.com>
>>>>>>>>>>>>>
>>>>>>>>>>>>> I think there's something wrong with your git scripts, I did
>>>>>>>>>>>>> not get this email.
>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Cc: Pavel Machek <pavel at denx.de>
>>>>>>>>>>>>>>>>>>> Cc: Marek Vasut <marex at denx.de>
>>>>>>>>>>>>>>>>>>> Cc: Wolfgang Denk <wd at denx.de>
>>>>>>>>>>>>>>>>>>> Cc: Stefan Roese <sr at denx.de>
>>>>>>>>>>>>>>>>>>> Cc: Tom Rini <trini at 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.
>
> Since this approach is not based on dwmmc TRM, it's based on SOCFPGA SoC,
> i think that it doesn't need to include into dwmmc core.
> If need to located into dwmmc core, this code needs to modify more.
>
> As drvsel and clksmpl are used at Exynos and SoCFPGA, it's not core feature.
> Other SoC can't be used them. we don't know.
>
>
Plus the way that this function is implemented, it is very specific to
SoCFGA, as the tables and rows are representing 45 degree increments for
the drvsel and smplsel value. Other platforms can have either more or
less granularity in the drvsel and smplsel values.
Dinh
More information about the U-Boot
mailing list