Re: [U-Boot] [PATCH 2/3] mmc: split fsl_esdhc driver for i.MX

14 Mar 2019

Hi Stefano,
...
-----Original Message-----
From: Stefano Babic sbabic@denx.de
Sent: Wednesday, March 13, 2019 7:53 PM
To: Y.b. Lu yangbo.lu@nxp.com; u-boot@lists.denx.de
Subject: Re: [U-Boot] [PATCH 2/3] mmc: split fsl_esdhc driver for i.MX
Hi Y.B lu,
On 21/02/19 08:55, Y.b. Lu wrote:
...
The fsl_esdhc driver was for Freescale eSDHC on MPC83XX/MPC85XX
initially. The later QoriQ series PowerPC processors (which were
evolutions of MPC83XX/MPC85XX), QorIQ series ARM processors, and i.MX
series processors were using this driver for their eSDHCs too.
For the two series processors, the eSDHCs are becoming more and more
different. We should have split it into two drivers, like them
(sdhci-of-esdhc.c/sdhci-esdhc-imx.c) in linux kernel.
This patch is just to create a fsl_esdhc_imx driver which is a copy of
fsl_esdhc driver for i.MX processors. We will convert i.MX processors
to use fsl_esdhc_imx, and clean up the two drivers separately in the
future patches.
Signed-off-by: Yangbo Lu yangbo.lu@nxp.com
drivers/mmc/Kconfig                          |  6 ++++++
 drivers/mmc/Makefile                         |  1 +
 drivers/mmc/{fsl_esdhc.c => fsl_esdhc_imx.c} |  5 +++--
 include/{fsl_esdhc.h => fsl_esdhc_imx.h}     | 11 ++++++-----
 4 files changed, 16 insertions(+), 7 deletions(-)  copy
drivers/mmc/{fsl_esdhc.c => fsl_esdhc_imx.c} (99%)  copy
include/{fsl_esdhc.h => fsl_esdhc_imx.h} (97%)
IMHO we can do better - if we split the code, we should be able to factorize
common code (if any) into a separate file and move the specific parts into
processor specific files. And at the same time, clean up what is not required
(for example, CONFIG_MCF5441x should not appear in imx code, and so on).
[Y.b. Lu] It's ideal to reuse common code. However I could find little which could be re-used.
As you see in the driver, there is conditional compiling in almost each function. (Some macros are specific for esdhc, and some are for esdhc_imx)
And the function is usually for a very basic function which is not worth to be split for common part.
Even some functions which are not in conditional compiling are specific for esdhc or esdhc_imx.
Besides, there will be two different registers structures for esdhc/esdhc_imx, since they are having more and more different registers and bits.
So I suggest just to split them, and there will be two totally different driver after cleaning up.
Regarding to MCF5441x, that's really a surprise to me. eSDHC actually is IP of only Freescale/NXP processors.
I don’t know any detail about it. But we can keep it in esdhc driver after splitting.
Do you think it's ok?
...
...

diff --git a/drivers/mmc/Kconfig b/drivers/mmc/Kconfig index
04a4e7716f..09bc02fe9c 100644
--- a/drivers/mmc/Kconfig
+++ b/drivers/mmc/Kconfig
@@ -641,6 +641,12 @@ config FSL_ESDHC
     This selects support for the eSDHC (enhanced secure digital host
     controller) found on numerous Freescale/NXP SoCs.
+config FSL_ESDHC_IMX

bool "Freescale/NXP i.MX eSDHC controller support"

We need a depend clause (ARCH_MX6, ARCH_MX5, ..)
[Y.b. Lu] Yes... Will add them.
...
...

help
 This selects support for the i.MX eSDHC (enhanced secure digital host


 controller) found on numerous Freescale/NXP SoCs.




endmenu
config SYS_FSL_ERRATUM_ESDHC111
diff --git a/drivers/mmc/Makefile b/drivers/mmc/Makefile index
7892c468f0..1287ad4cc1 100644
--- a/drivers/mmc/Makefile
+++ b/drivers/mmc/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_MMC_DW_K3)			+=
hi6220_dw_mmc.o
...
obj-$(CONFIG_MMC_DW_ROCKCHIP)		+= rockchip_dw_mmc.o
 obj-$(CONFIG_MMC_DW_SOCFPGA)		+= socfpga_dw_mmc.o
 obj-$(CONFIG_FSL_ESDHC) += fsl_esdhc.o
+obj-$(CONFIG_FSL_ESDHC_IMX) += fsl_esdhc_imx.o
 obj-$(CONFIG_FTSDC010) += ftsdc010_mci.o
 obj-$(CONFIG_GENERIC_ATMEL_MCI) += gen_atmel_mci.o
 obj-$(CONFIG_MMC_MESON_GX) += meson_gx_mmc.o diff --git
a/drivers/mmc/fsl_esdhc.c b/drivers/mmc/fsl_esdhc_imx.c similarity
index 99% copy from drivers/mmc/fsl_esdhc.c copy to
drivers/mmc/fsl_esdhc_imx.c index 21fa2ab1d4..9c823e86e2 100644
--- a/drivers/mmc/fsl_esdhc.c
+++ b/drivers/mmc/fsl_esdhc_imx.c
@@ -2,6 +2,7 @@
 /*

Copyright 2007, 2010-2011 Freescale Semiconductor, Inc
Andy Fleming



Copyright 2019 NXP

Based vaguely on the pxa mmc code:
(C) Copyright 2003



@@ -18,7 +19,7 @@
 #include <part.h>
 #include <power/regulator.h>
 #include <malloc.h>
-#include <fsl_esdhc.h>
+#include <fsl_esdhc_imx.h>
 #include <fdt_support.h>
 #include <asm/io.h>
 #include <dm.h>
@@ -110,7 +111,7 @@ struct esdhc_soc_data {

@non_removable: 0: removable; 1: non-removable
@wp_enable: 1: enable checking wp; 0: no check
@vs18_enable: 1: use 1.8V voltage; 0: use 3.3V



@flags: ESDHC_FLAG_xx in include/fsl_esdhc.h





@flags: ESDHC_FLAG_xx in include/fsl_esdhc_imx.h
@caps: controller capabilities
@tuning_step: tuning step setting in tuning_ctrl register
@start_tuning_tap: the start point for tuning in tuning_ctrl



register diff --git a/include/fsl_esdhc.h b/include/fsl_esdhc_imx.h
similarity index 97% copy from include/fsl_esdhc.h copy to
include/fsl_esdhc_imx.h index 8dbd5249a7..e05b24e7e8 100644
--- a/include/fsl_esdhc.h
+++ b/include/fsl_esdhc_imx.h
@@ -4,10 +4,11 @@
  *-------------------------------------------------------------------


Copyright 2007-2008,2010-2011 Freescale Semiconductor, Inc



Copyright 2019 NXP

*/

-#ifndef  __FSL_ESDHC_H__
-#define	__FSL_ESDHC_H__
+#ifndef __FSL_ESDHC_IMX_H__
+#define __FSL_ESDHC_IMX_H__
#include <linux/bitops.h>
 #include <linux/errno.h>
@@ -258,15 +259,15 @@ struct fsl_esdhc_cfg {  #error "Endianess is not
defined: please fix to continue"
 #endif
-#ifdef CONFIG_FSL_ESDHC
+#ifdef CONFIG_FSL_ESDHC_IMX
 int fsl_esdhc_mmc_init(bd_t *bis);
 int fsl_esdhc_initialize(bd_t *bis, struct fsl_esdhc_cfg *cfg);  void
fdt_fixup_esdhc(void *blob, bd_t *bd);  #else  static inline int
fsl_esdhc_mmc_init(bd_t *bis) { return -ENOSYS; }  static inline void
fdt_fixup_esdhc(void *blob, bd_t *bd) {} -#endif /* CONFIG_FSL_ESDHC
*/
+#endif /* CONFIG_FSL_ESDHC_IMX */
 void __noreturn mmc_boot(void);
 void mmc_spl_load_image(uint32_t offs, unsigned int size, void
*vdst);
-#endif  /* __FSL_ESDHC_H__ */
+#endif  /* __FSL_ESDHC_IMX_H__ */
I tend to go further and to cleanup the new file removing what is not required.
Do it in a separate patch, but belonging to the same series, so that is easier to
review - thanks !
[Y.b. Lu] I assume you meant both driver and header files.
That will take some time for me.
...
Best regards,
Stefano Babic
--
==
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic@denx.de
===================================================================
==

    