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

[Marek Vasut]

On 1/2/19 11:31 AM, Lukasz Majewski wrote:
> On Wed, 2 Jan 2019 02:18:58 +0100
> Marek Vasut <marex at 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 at 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).

Well fix the clock code then ? All the other subsystems use the
SEQ_ALIAS and DT /aliases node to define ordering, I don't see why FSL
SDHC driver should get some special hacky treatment which will only make
it difficult to maintain in the future .

-- 
Best regards,
Marek Vasut