[U-Boot] [PATCH v1 12/14] imx: mmc: Use 'fsl, usdhc-index' property to provide esdhc controller number

[Lukasz Majewski <lukma@denx.de>]

On Wed, 2 Jan 2019 02:18:58 +0100
Marek Vasut <marex@denx.de> wrote:

> On 1/2/19 12:37 AM, Lukasz Majewski wrote:
> > With the current code, it is not possible to assign different than
> > default numbers for mmc controllers.
> > 
> > Several in-tree boards depend on the pre-dm setup, corresponding to
> > following aliases:
> > 
> > mmc0 = &usdhc2; --> fsl,usdhc-index = <1>
> > mmc1 = &usdhc4; --> fsl,usdhc-index = <3>
> > 
> > Without this patch we are either forced to use default aliasing -
> > like:
> > 
> > mmc0 = &usdhc1;
> > mmc1 = &usdhc2;
> > mmc2 = &usdhc3;
> > mmc3 = &usdhc4;
> > 
> > to have the proper clocks setup for the controller. However, such
> > setup is not acceptable for some legacy scripts / code.
> > 
> > With this patch - by introducing 'fsl,usdhc-index' - one can
> > configure (get) clock rate corresponding to used controller.
> > 
> > Moreover, as this code is used in the SPL before relocation (and to
> > save space we strip the SPL DTS from clocks and its names) adding
> > separate properties seems to be the best approach here. One also
> > avoids adding clocks DM code to SPL.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@denx.de>
> > ---
> > 
> >  drivers/mmc/fsl_esdhc.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc.c
> > index 3cdfa7f5a6..49a6834a98 100644
> > --- a/drivers/mmc/fsl_esdhc.c
> > +++ b/drivers/mmc/fsl_esdhc.c
> > @@ -1401,6 +1401,7 @@ static int fsl_esdhc_probe(struct udevice
> > *dev) fdt_addr_t addr;
> >  	unsigned int val;
> >  	struct mmc *mmc;
> > +	int usdhc_idx;
> >  	int ret;
> >  
> >  	addr = dev_read_addr(dev);
> > @@ -1513,7 +1514,21 @@ static int fsl_esdhc_probe(struct udevice
> > *dev) 
> >  		priv->sdhc_clk = clk_get_rate(&priv->per_clk);
> >  	} else {
> > -		priv->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK +
> > dev->seq);
> > +		/*
> > +		 * Check for 'fsl,index' DTS property - as one may
> > want to have
> > +		 * following mmc setup:  
> 
> NAK, DT is a hardware description. This is encoding a policy, which
> should not be in DT.

Please look a few lines up in this file:

mmc0 = &usdhc1;
mmc1 = &usdhc2;
mmc2 = &usdhc3;
mmc3 = &usdhc4;

The fsl_esdhc.c has hardcoded ordering for eMMC devices when
setting/getting clock. 

If you change aliases on your dts (mmc0 -> usdhc2, etc). Then with a
bit of luck your second controller will be initialized with first's one
clock value :-). This of course works by chance with default ROM setup.

The problem is that many boards have different mmc ordering (starting
with mmc0, which in above scheme is usdhc2 controller. Also mmc1 is the
usdhc4 to which eMMC is connected in many boards). Of course this could
be changed, but please consider a lot of legacy code pilling up on the
customer's side.

The clock index @ (drivers/mmc/fsl_esdhc.c - L1517):
mxc_get_clock(MXC_ESDHC_CLK + dev->seq);

Here the dev->seq is 0,1 (with SEQ_ALIAS), which will provide clock
values from usdhc1 and usdhc2. However, we use usdhc4 (eMMC) and usdhc2
(SD).

> 
> This looks like some reimplementation of SEQ_ALIAS stuff.

No, this is a fix for hardcoded (assumed) clock setup in this driver.

The other option is to provide/port clock stuff from linux (and
implement CLK_DM in u-boot at least for this part). However, this will
not fix the problem described above (for other boards which use the
"legacy" approach).

> 
> > +		 * mmc0 = &usdhc2; --> fsl,index = <1>
> > +		 * mmc1 = &usdhc4; --> fsl,index = <3>
> > +		 *
> > +		 * So we do have dev->seq = {0, 1}, which in the
> > below code
> > +		 * doesn't correspond to correct USDHC clocks.
> > +		 *
> > +		 * For that reason a new "fsl,index" property has
> > been
> > +		 * introduced.
> > +		 */
> > +		usdhc_idx = dev_read_u32_default(dev,
> > "fsl,usdhc-index",
> > +						 dev->seq);
> > +		priv->sdhc_clk = mxc_get_clock(MXC_ESDHC_CLK +
> > usdhc_idx); if (priv->sdhc_clk <= 0) {
> >  			dev_err(dev, "Unable to get clk for %s\n",
> > dev->name); return -EINVAL;
> >   
> 
> 




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190102/c53a6fc5/attachment.sig>