On Tuesday, December 01, 2015 at 04:32:44 PM, Chin Liang See wrote:
> Hi Marek,
Hi!
> On Sun, 2015-11-29 at 14:39 +0100, Marek Vasut wrote:
> > On Friday, November 27, 2015 at 08:22:03 AM, Chin Liang See wrote:
> > > Enable SDMMC calibration to determine the best setting for
> > > drvsel and smplsel. Calibration will be triggered if the
> > > drvsel and smplsel node are not available in DTS.
> > >
> > > Signed-off-by: Chin Liang See <cl...@altera.com>
> > > Cc: Dinh Nguyen <dingu...@opensource.altera.com>
> > > Cc: Dinh Nguyen <dinh.li...@gmail.com>
> > > Cc: Pavel Machek <pa...@denx.de>
> > > Cc: Marek Vasut <ma...@denx.de>
> > > Cc: Stefan Roese <s...@denx.de>
> > > Cc: Pantelis Antoniou <pantelis.anton...@konsulko.com>
> > > Cc: Simon Glass <s...@chromium.org>
> > > Cc: Jaehoon Chung <jh80.ch...@samsung.com>
> > > ---
> > > Changes for v4
> > > - Calibration only run if node not in DTS
> > > Changes for v3
> > > - Remove the && ok as its redundant
> > > Changes for v2
> > > - Using standard error return macro
> > > - Split to small function to avoid deep identation
> > > - Fix coding standard
> > > ---
> > >
> > > drivers/mmc/socfpga_dw_mmc.c | 208
> > >
> > > ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 205
> > > insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/mmc/socfpga_dw_mmc.c
> > > b/drivers/mmc/socfpga_dw_mmc.c
> > > index 2bd0ebd..a8e2660 100644
> > > --- a/drivers/mmc/socfpga_dw_mmc.c
> > > +++ b/drivers/mmc/socfpga_dw_mmc.c
> > > @@ -13,6 +13,7 @@
> > >
> > > #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;
> > >
> > > @@ -25,7 +26,144 @@ struct dwmci_socfpga_priv_data {
> > >
> > > unsigned int smplsel;
> > >
> > > };
> > >
> > > -static void socfpga_dwmci_clksel(struct dwmci_host *host)
> > > +/*
> > > + * rows and columns of calibration rectange. The values are based
> > > on the
> > > value + * range of drvsel and smplsel register in system manager.
> > > Note
> > > drvsel 0 is + * unusable as it has meta-stability issue.
> > > + */
> > > +#define SOCFPGA_SD_DRVSEL 7
> > > +#define SOCFPGA_SD_SMPLSEL 8
> > > +
> > > +static int socfpga_dwmci_find_row_col_fit_rectangle(unsigned
> > > rect_width,
> > > + unsigned rect_height,
> > > + unsigned char
> > > cal_results[SOCFPGA_SD_DRVSEL][SOCFPGA_SD_SMPLSEL],
> > > + unsigned int *cal_row, unsigned int *cal_col)
> > > +{
> > > + unsigned char start_row, start_col;
> > > +
> > > + /* Find the row and column where the candidate fits */
> > > + for (start_col = 0; start_col < (SOCFPGA_SD_SMPLSEL -
> > > rect_width + 1);
> > > + start_col++) {
> > > + for (start_row = 0;
> > > + start_row < (SOCFPGA_SD_DRVSEL - 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);
> > > 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
> > > + */
> >
> > I think you can refactor this indentation horror some more, right ?
>
> Ok, let me use shorter variable to avoid going next line.
>
> > > + 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;
> >
> > For example this condition can be inverted and it'd shave off one
> > level
> > of indent.
>
> Ok let me take a look when doing the house keeping
That's really help, thanks/
> > > + }
> > > + }
> > > + }
> > > + return -EINVAL;
> > > +}
[...]
> > > @@ -102,8 +304,8 @@ static int socfpga_dwmci_of_probe(const void
> > > *blob, int
> > > node, const int idx) host->bus_hz = clk;
> > >
> > > host->fifoth_val = MSIZE(0x2) |
> > >
> > > RX_WMARK(fifo_depth / 2 - 1) | TX_WMARK(fifo_depth
> > >
> > > / 2);
> > > - priv->drvsel = fdtdec_get_uint(blob, node, "drvsel", 3);
> > > - priv->smplsel = fdtdec_get_uint(blob, node, "smplsel", 0);
> > > + priv->drvsel = fdtdec_get_uint(blob, node, "drvsel", 0xf);
> > > + priv->smplsel = fdtdec_get_uint(blob, node, "smplsel",
> > > 0xf);
> >
> > This breaks multiple boards, since it misconfigures the default
> > values.
>
> The 0xf is non valid value. When the node is missing, we will get 0xf
> during the probe. When the init start, the non valid value will trigger
> the calibration to get the correct value.
OK, this is bad. Originally, if we didn't specify these in the DT, we would
use the default values of 0x3 and 0x0 , but now we do the calibration. I wonder,
do we care about DT ABI compatibility on the U-Boot level or not ?
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
