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

Chin Liang See
2015-09-01T08:54:06Z

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

Thanks
Chin Liang

>
> >
> > >
> > > While finding the value, one of our customer have this issue. The SDMMC
> > > communication doesn't work when they route the SDMMC slot few inches
> > > away from the chip and running at high frequency.
> >
> > High frequency? DDR50 mode? I know that it's not implemented for the upper mode at bootloader.
> > Is it related with controlling power, not controlling cmu?
>
> Actually I am comparing the enumeration and normal run (SDR25). The
> failure is spotted during data transfer such as fatload. Wonder what is
> cmu? :)
>
> Thanks
> Chin Liang
>
>
> >
> > Best Regards,
> > Jaehoon Chung
> >
> > >
> > > With that, I shall introduce the code into socfpga_dw_mmc instead common
> > > dw_mmc then. Wonder you would agree with that?
> > >
> > > Thanks
> > > Chin Liang
> > >
> > >
> > >> Best Regards,
> > >> Jaehoon Chung
> > >>
> > >>>>
> > >>>> Thanks for notifying this. But I can't find this patch at patchwork.
> > >>>> I'm not reading fully mail thread, so after reading mails, i will resend at mailing list.
> > >>>
> > >>> Sure, here is the link :)
> > >>> http://lists.denx.de/pipermail/u-boot/2015-August/224566.html
> > >>>
> > >>> Thanks
> > >>> Chin Liang
> > >>>
> > >>>
> > >>>>
> > >>>> Best Regards,
> > >>>> Jaehoon Chung
> > >>>>
> > >>>>>
> > >>>>> Jaehoon, this patch will do a calibration to determine the clksel value.
> > >>>>> Wonder is this something would help or applicable for Exynos? We are
> > >>>>> probing whether this code should go dw_mmc.c instead socfpga_dw_mmc.c.
> > >>>>>
> > >>>>> Thanks
> > >>>>> Chin Liang
> > >>>>>
> > >>>>>> Regards,
> > >>>>>> Simon
> > >>>>>
> > >>>>>
> > >>>>>
> > >>>>
> > >>>
> > >>>
> > >>> _______________________________________________
> > >>> U-Boot mailing list
> > >>> U-Boot at lists.denx.de
> > >>> http://lists.denx.de/mailman/listinfo/u-boot
> > >>>
> > >>
> > >
> > >
> > >
> >
>