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

Simon Glass
2015-08-21T20:52:20Z

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

Regards,
Simon