On Tue, 2015-12-01 at 16:38 +0100, Marek Vasut wrote:
> 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 ?
If the compatibility failed, it will still invoke the calibration to
get the correct value. Just that its in the cost of boot time.
At same time, I am rethinking another email thread on suggesting to put
this into common dw_mmc code. Here are my proposal
1. Put back the correct default value for DT ABI compatiblity
2. Adding a node to enable calibration. If node not exist, calibration
will disabled as default.
3. Some common calibration algo will go into dw_mmc.c
4. platform specific such as on configuring the clksel and smplsel will
be part of platform specific dw_mmc file such as socfpga_dw_mmc.c
This hope will work for both everyone especially for socfpga and exynos
platform. Any thoughts?
Thanks
Chin Liang
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
